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: Rerun selection if state changed during mount #11777

Merged
merged 1 commit into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Nov 12, 2018

This pull request seeks to resolve an issue with the @wordpress/data withSelect higher-order component which would have prevented a wrapped component from receiving the most up-to-date store values if it had incurred a store state change during its own mount (e.g. dispatching during its own constructor).

Testing instructions:

Ensure unit tests pass:

npm run test-unit packages/data

cc @jsnajdr @Tug

@youknowriad

👍

@youknowriad youknowriad added this to the 4.4 milestone Nov 13, 2018

@Tug

Tug approved these changes Nov 13, 2018

I did try it against https://github.com/wordpress-mobile/gutenberg-mobile/tree/add/gb-store-test and it fixed the issue 👍

@@ -48,13 +48,23 @@ const withSelect = ( mapSelectToProps ) => createHigherOrderComponent( ( Wrapped
constructor( props ) {
super( props );
this.onStoreChange = this.onStoreChange.bind( this );
this.subscribe( props.registry );

This comment has been minimized.

@jsnajdr

jsnajdr Nov 13, 2018

Contributor

Subscribing in constructor can cause subscriptions to leak in two cases:

  • when server-side rendering, constructor is called, but componentWillUnmount never is.
  • in React Fiber concurrent mode, component instances can be constructed, but never actually committed to DOM. This can happen when Fiber is working through rendering a low-priority update (calling constructors and render methods) and suddenly a higher priority update is scheduled that invalidates the work that's been done. Then the unfinished low-priority work will be thrown away and restarted. In such a case, constructor is called, but the component(DidMount|WillUnmount) methods aren't.

The correct place to subscribe is therefore always componentDidMount.

Then, of course, we might miss a store update that happened between constructor and componentDidMount.

react-redux solves that by rerunning the selector (i.e., mapStateToProps) in componentDidMount and calling forceUpdate if anything changed. That's the stable v5.1.1.

In master, they have an unreleased beta version where the Redux provider and consumer are rewritten using the new React context API and where the connect-ed consumers don't subscribe to the store at all. Just the Provider does. That could be an inspiration for the @wordpress/data internals, too 🙂

Conclusion: this patch solves the bug that @Tug discovered, but other subtle issues are still present.

This comment has been minimized.

@youknowriad

youknowriad Nov 13, 2018

Contributor

Subscribing in the constructor is important for another use-case. It's important that the parent's component rerenders before its children do. For example when you remove a block, you don't want its inner components to rerender, you want the block list to rerender first to remove the block before the selectors of a removed block get called.

There's probably a better way to fix it but moving to componentDidMount causes the components to rerender in the wrong order because componentDidMount of children components runs before parents. I wonder how connect solves this.

This comment has been minimized.

@Tug

Tug Nov 13, 2018

Contributor

Found a comment that I believe explains it: reduxjs/react-redux#1079 (comment)
Makes me wonder if we should use a per component listener?

This comment has been minimized.

@jsnajdr

jsnajdr Nov 13, 2018

Contributor

In react-redux, nested connected components don't subscribe directly to the Redux store, but there is a nested hierarchy of Subscription objects and each child component subscribes to the parent's Subscription. Only components that don't have any connected parent subscribe directly to the store. Then the Subscription object makes sure that the listener of its owner is called first, and the listeners of the children are called after it. Something like this:

constructor() {
  this.subscription = new Subscription();
  // `this` is always the first subscriber
  this.subscription.subscribe( this.onStateChange ); 
}

componentDidMount() {
  this.subscription.subscribeToParent();
}

componentDidUnmount() {
  this.subscription.unsubscribeFromParent();
}

The parent subscription object is passed down to children in context. And then each child in turn passes a different parent subscription to its children.

It's very clever and satisfies all the subtle requirements: no leaks, parent listener called before children...

This comment has been minimized.

@youknowriad

youknowriad Nov 13, 2018

Contributor

I'd be fine with something like this if proposed, seems like a good enhancement.

This comment has been minimized.

@jsnajdr

jsnajdr Nov 13, 2018

Contributor

The latest React Redux 6 beta is even better. It uses React.createContext and only the Provider subscribes to anything. The connected components receive the latest Redux state through the context and rerender if anything relevant changed. Parent component receives the updated context before children do. Everything is much simpler and it just works.

This comment has been minimized.

@aduth

aduth Nov 13, 2018

Member

I'm interested in exploring further enhancements, and have a few ideas floating in my head related to what's been discussed here. To clarify, do these concerns impact whether this should be merged as-is?

This comment has been minimized.

@jsnajdr

jsnajdr Nov 13, 2018

Contributor

To clarify, do these concerns impact whether this should be merged as-is?

No. As far as I know, this PR makes things better and doesn't make anything worse 🚢

This comment has been minimized.

@aduth

aduth Nov 13, 2018

Member

Gotcha, great. And thanks for the additional context. While I've done my fair share of scouring react-redux source, in how you've laid it out here, there seems to be much we could still do to improve. I'll plan to dig deeper very shortly 🙂

@Tug Tug referenced this pull request Nov 13, 2018

Merged

Use Gutenberg store #226

@aduth aduth merged commit fba803e into master Nov 13, 2018

1 check passed

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

@aduth aduth deleted the fix/with-select-constructor-dispatch branch Nov 13, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment