Skip to content
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

Merged
merged 5 commits into from
Jan 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 7.1.0 (Unreleased)

### Improvements

- `withFilters` has been optimized to avoid binding hook handlers for each mounted instance of the component, instead using a single centralized hook delegator.
- `withFilters` has been optimized to reuse a single shared component definition for all filtered instances of the component.

## 7.0.5 (2019-01-03)

## 7.0.4 (2018-12-12)
Expand Down
113 changes: 81 additions & 32 deletions packages/components/src/higher-order/with-filters/index.js
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
Expand All @@ -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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, if this is meant to account for new filters applied after the initial render, isn't it missing a critical piece of calling applyFilters again after those new filter handlers are added / removed?

@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 the throttledForceUpdate method is that it will be executed only once when multiple calls of add/remove filters occurs in the same small period of time.

Copy link
Member Author

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.

Yes, I deleted it once I noticed I'd overlooked the re-assignment in the debounce callback.

The benefit of calling applyFilters inside of the throttledForceUpdate method is that it will be executed only once when multiple calls of add/remove filters occurs in the same small period of time.

Yeah, I can see that being a good thing. It might require that we do the refactoring I'd pondered at #13231 (comment) ?

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might trigger twice:

  • when incrementing the number of instances
  • when decrementing the number of instances

It should only be executed when the number of instances changes from 0 to 1.

Copy link
Member

@gziolo gziolo Jan 8, 2019

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

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' );
}