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: Avoid responding to `componentDidUpdate` in `withDispatch` #11866

Merged
merged 2 commits into from Nov 19, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented Nov 14, 2018

Related (alternative to): #11870

Note: The changes are less overwhelming to view when ignoring whitespace changes.

This pull request seeks to explore an optimization to withDispatch to avoid running shallow equality comparison on incoming props (via pure) and in re-assigning "proxy props" at its componentDidUpdate lifecycle. The caveat here is that mapDispatchToProps must always return a consistent set of object keys.

Implementation notes:

Technically this is very much broken in the current implementation of the component anyways, as the proxy props are only reassigned after the component updates. Thus, the underlying component would not receive the changed prop keys from the new mapDispatchToProps return value.

The main advantage here is that mapDispatchToProps only needs to be called once at component mount and once for each dispatch which occurs. These are significantly fewer occurrences than those under which the props received by withDispatch were to change (even whilst pure).

In a separate approach, I may consider to see how to respect changing incoming props while limiting one or both of (a) calls to mapDispatchToProps and (b) iterating to create a set of proxyProps.

Testing instructions:

Ensure tests pass:

npm test
@@ -27,63 +27,57 @@ import { RegistryConsumer } from '../registry-provider';
* @return {Component} Enhanced component with merged dispatcher props.
*/
const withDispatch = ( mapDispatchToProps ) => createHigherOrderComponent(
compose( [
pure,

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

Any reason to remove pure?

This comment has been minimized.

@aduth

aduth Nov 14, 2018

Member

Any reason to remove pure?

The thought was since we no longer respond to props changes, nor perform any expensive operations in the render, they could just go straight through.

The main downside I might see is if someone had come to rely on withDispatch for its pure behavior, which is and always would be a fragile dependency.

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

I'm fine with this PR. Ultimately, I think we should try to find a way to measure these PRs somehow, I know it's very difficult to do and I'd appreciate thoughts on that as I want to measure my own performance PRs as well.

@aduth

This comment has been minimized.

Member

aduth commented Nov 15, 2018

This and #11870 could be considered competing approaches. Do you have any preference one way or another? I think this branch is a bigger performance win, but comes at the expense of some developer ergonomics (inability to conditionally return different keys from withDispatch).

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

I prefer #11870 because of its simplicity. My hesitation is the performance impact cause I don't know how to measure it.

@aduth

This comment has been minimized.

Member

aduth commented Nov 15, 2018

I think #11870 is a bigger performance win,

There was a typo here. This pull request is a bigger performance win.

In case that makes any difference.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

It does, let's go with the most performant one as this is a critical path.

@aduth aduth added this to the 4.5 milestone Nov 15, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

@aduth Feel free to merge the most performant PR between the two :)

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

Going to go with this one. It's a bit more unintuitive in the chance a developer wants to return a separate set of props, but given that the behavior has been largely broken anyways since introduced, and we had no occurrences of it in our code, and it's forward-compatible to reintroduce support, and above all the significant performance win, I think it's a worthwhile compromise.

@aduth aduth merged commit 229ed1c into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the try/with-dispatch-ignore-new-props branch Nov 19, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Data: Avoid responding to `componentDidUpdate` in `withDispatch` (Wor…
…dPress#11866)

* Data: Avoid responding to `componentDidUpdate` in `withDispatch`

* Data: Update withSelect test case for ignored select props changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment