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

Data: Refactor and optimize withSelect, withDispatch handling of registry change #11027

Merged
merged 4 commits into from
Oct 26, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 24, 2018

This pull request seeks to refactor the implementations of withSelect and withDispatch to avoid dependency upon -- and thus enable the deprecation of -- the remountOnPropChange higher-order component.

A few notes as to the rationale:

  • remountOnPropChange used broadly outside this specific context could serve as a "footgun", since it can have negative effects in focus loss (example)
  • Higher-order components are considered to be performance-impacting (reference). withSelect and withDispatch are two of the hottest code paths, and as implemented prior to these changes would each have a remountOnPropChange wrapper
  • The remountOnPropChange wrapper for withDispatch is actually not necessary, due to its behavior of proxying function calls to resolve registry.dispatch only when the mapped prop is called.

Testing Instructions:

This is a refactoring change. There should be no impact on usage.

Ensure unit tests pass:

npm run test-unit packages/data

@aduth aduth added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality [Package] Data /packages/data labels Oct 24, 2018
const hasRegistryChanged = nextProps.registry !== this.props.registry;
if ( hasRegistryChanged ) {
this.unsubscribe();
this.subscribe( nextProps.registry );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that we're doing this magic in shouldComponentUpdate. It doesn't seem like the best function for this kind of side effects. Ideally it's componentWillReceiveProps but this one has been deprecated, so not sure what's the "good" approach for these at the moment.

I see shouldComponentUpdate as a function that can be called multiple times by React even without rendering (especially in async) as it's supposed to be a pure function just returning a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

My general approach with these components has been to happily throw out all best practices if it means performance could suffer even marginally in the individual case by following them.

If there's an unexpected behavior that could occur in how we're implementing (even "number of calls"), we should have a unit test to cover it in whatever fix need to be made.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍 The e2e tests spoke for this :P

There's a small unit test breakage to fix and rebase

@aduth aduth force-pushed the remove/remount-on-prop-change branch from f14e050 to 39a7399 Compare October 26, 2018 13:09
@aduth
Copy link
Member Author

aduth commented Oct 26, 2018

Pushed rebased branch.

Including two issues to have been resolved:

  • Remove remaining unused reference to remountOnPropChange in with-dispatch/index.js
  • Update tests for remountOnPropChange to expect the deprecated call

@aduth aduth merged commit 1e53293 into master Oct 26, 2018
@aduth aduth deleted the remove/remount-on-prop-change branch October 26, 2018 13:25
@mtias mtias added this to the 4.2 milestone Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants