-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Components: Optimize withFilters, avoiding per-instance bindings #13231
Changes from all commits
6090d1f
678ade4
fb7fbe2
81a81ad
274f516
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { debounce, uniqueId } from 'lodash'; | ||
import { debounce, without } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
|
@@ -24,45 +24,94 @@ const ANIMATION_FRAME_PERIOD = 16; | |
*/ | ||
export default function withFilters( hookName ) { | ||
return createHigherOrderComponent( ( OriginalComponent ) => { | ||
return class FilteredComponent extends Component { | ||
/** @inheritdoc */ | ||
constructor( props ) { | ||
super( props ); | ||
|
||
this.onHooksUpdated = this.onHooksUpdated.bind( this ); | ||
this.Component = applyFilters( hookName, OriginalComponent ); | ||
this.namespace = uniqueId( 'core/with-filters/component-' ); | ||
this.throttledForceUpdate = debounce( () => { | ||
this.Component = applyFilters( hookName, OriginalComponent ); | ||
this.forceUpdate(); | ||
}, ANIMATION_FRAME_PERIOD ); | ||
|
||
addAction( 'hookRemoved', this.namespace, this.onHooksUpdated ); | ||
addAction( 'hookAdded', this.namespace, this.onHooksUpdated ); | ||
const namespace = 'core/with-filters/' + hookName; | ||
|
||
/** | ||
* The component definition with current filters applied. Each instance | ||
* reuse this shared reference as an optimization to avoid excessive | ||
* calls to `applyFilters` when many instances exist. | ||
* | ||
* @type {?Component} | ||
*/ | ||
let FilteredComponent; | ||
|
||
/** | ||
* Initializes the FilteredComponent variable once, if not already | ||
* assigned. Subsequent calls are effectively a noop. | ||
*/ | ||
function ensureFilteredComponent() { | ||
if ( FilteredComponent === undefined ) { | ||
FilteredComponent = applyFilters( hookName, OriginalComponent ); | ||
} | ||
} | ||
|
||
/** @inheritdoc */ | ||
componentWillUnmount() { | ||
this.throttledForceUpdate.cancel(); | ||
removeAction( 'hookRemoved', this.namespace ); | ||
removeAction( 'hookAdded', this.namespace ); | ||
class FilteredComponentRenderer extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
ensureFilteredComponent(); | ||
} | ||
|
||
componentDidMount() { | ||
FilteredComponentRenderer.instances.push( this ); | ||
|
||
// If there were previously no mounted instances for components | ||
// filtered on this hook, add the hook handler. | ||
if ( FilteredComponentRenderer.instances.length === 1 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might trigger twice:
It should only be executed when the number of instances changes from 0 to 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking a bit more about it, it is probably correct. I mean, that when mounting it always increments the counter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, being that it only increments here, I think it should be fine as it is. |
||
addAction( 'hookRemoved', namespace, onHooksUpdated ); | ||
addAction( 'hookAdded', namespace, onHooksUpdated ); | ||
} | ||
} | ||
|
||
/** | ||
* When a filter is added or removed for the matching hook name, the wrapped component should re-render. | ||
* | ||
* @param {string} updatedHookName Name of the hook that was updated. | ||
*/ | ||
onHooksUpdated( updatedHookName ) { | ||
if ( updatedHookName === hookName ) { | ||
this.throttledForceUpdate(); | ||
componentWillUnmount() { | ||
FilteredComponentRenderer.instances = without( | ||
FilteredComponentRenderer.instances, | ||
this | ||
); | ||
|
||
// If this was the last of the mounted components filtered on | ||
// this hook, remove the hook handler. | ||
if ( FilteredComponentRenderer.instances.length === 0 ) { | ||
removeAction( 'hookRemoved', namespace ); | ||
removeAction( 'hookAdded', namespace ); | ||
} | ||
} | ||
|
||
/** @inheritdoc */ | ||
render() { | ||
return <this.Component { ...this.props } />; | ||
return <FilteredComponent { ...this.props } />; | ||
} | ||
} | ||
|
||
FilteredComponentRenderer.instances = []; | ||
|
||
/** | ||
* Updates the FilteredComponent definition, forcing a render for each | ||
* mounted instance. This occurs a maximum of once per animation frame. | ||
*/ | ||
const throttledForceUpdate = debounce( () => { | ||
// Recreate the filtered component, only after delay so that it's | ||
// computed once, even if many filters added. | ||
FilteredComponent = applyFilters( hookName, OriginalComponent ); | ||
|
||
// Force each instance to render. | ||
FilteredComponentRenderer.instances.forEach( ( instance ) => { | ||
instance.forceUpdate(); | ||
} ); | ||
}, ANIMATION_FRAME_PERIOD ); | ||
|
||
/** | ||
* When a filter is added or removed for the matching hook name, each | ||
* mounted instance should re-render with the new filters having been | ||
* applied to the original component. | ||
* | ||
* @param {string} updatedHookName Name of the hook that was updated. | ||
*/ | ||
function onHooksUpdated( updatedHookName ) { | ||
if ( updatedHookName === hookName ) { | ||
throttledForceUpdate(); | ||
} | ||
}; | ||
} | ||
|
||
return FilteredComponentRenderer; | ||
}, 'withFilters' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth, I saw this question in my inbox. I can't see it anymore in the original PR. I just wanted to confirm that it is triggered here and in your optimized version properly.
The benefit of calling
applyFilters
inside of thethrottledForceUpdate
method is that it will be executed only once when multiple calls of add/remove filters occurs in the same small period of time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I deleted it once I noticed I'd overlooked the re-assignment in the debounce callback.
Yeah, I can see that being a good thing. It might require that we do the refactoring I'd pondered at #13231 (comment) ?