connect(mapStateToProps) is executed with component props based on the previous state

[esamattis]

Ok, this is really hard to explain so I've set up an example here:

https://github.com/epeli/redux-connect-demo

Build it and play with it with devtools console opened. You should see following errors:

Uncaught TypeError: Cannot read property 'foo' of undefined

This happens because the mapStateToProps of the Item component is called with component props based on the previous store state. The two states are not compatible anymore and it causes all kinds of errors.

I think the parent should render and update the props for the Item component before the mapStateToProps of the Item component can be executed.

[gaearon]

This happens because the mapStateToProps of the Item component is called with component props based on the previous store state.

Why do you say the previous state? I think they are called with the most recent state—and because the Items have not yet unmounted, they query state that doesn't exist anymore.

I don't think this is something we can fix. React state changes are asynchronous and React may (or may not) batch them. Therefore, the moment you press “Remove”, the Redux store updates, and both Item and App receive the new state. Even if the App state change results in unmounting of Items, that will happen later than mapStateToProps is called for Item.

Unless I'm mistaken, there is nothing we can do. You have two options:

1. Request all required state at App (or a lower, e.g. ItemList) level and pass it down to “dumb” Items.
2. Add safeguards to mapStateToProps for “currently unmounting” state. For example, you may return null from render in this case.

Potentially we could have the component generated by connect() return null from its render if mapStateToProps returned null. Does this make any sense? Is this too surprising?

[esamattis]

Why do you say the previous state?

Because the store has been updated and the props App has passed to the Item are based on an old store state at the time the store query is run.

Therefore, the moment you press “Remove”, the Redux store updates, and both Item and App receive the new state.

Hmm. Is it actually required for the both to be subscribed to the store? Wouldn't it be enough if only the one most closest to the component root is subscribed? When that component renders it will trigger a render (and a store query) in all child components.

Seems like it would solve this. Although I have no idea how to detect the one closest to the root.

[esamattis]

Darn. Just realized it would break apps using pure components. Because the props between the parent and child components does not necessary change it might prevent some children from updating because React thinks there's nothing to the update but there might be because the store has changed.

[esamattis]

Potentially we could have the component generated by connect() return null from its render if mapStateToProps returned null. Does this make any sense? Is this too surprising?

I find it really counter intuitive that you have consider state transitions in the store query. This really feels a bug to me but I see how it's unresolvable with the current React APIs.

When thinking about the roots of this issue I think it can be nailed down to the fact that connect can be used in a way to break the unidirectional data flow. It can inject new data to multiple points in the component tree and when you do that you will have a situation where you have some stale data between the tree nodes.

1. Request all required state at App (or a lower, e.g. ItemList) level and pass it down to “dumb” Items.

This seems to be the right way to do it. Although I sometimes feel that I have to introduce new props to intermediate components just for them to passing those down. Which feels ugly. But I'm not sure it really is because it solves so many issues and weird edge cases like this.

Could Redux just prevent people from ending up with this situation? Maybe by removing ownProps param from mapStateToProps? It took me quite a while to figure out what the hell was going on.

[gaearon]

Could Redux just prevent people from ending up with this situation? Maybe by removing ownProps param from mapStateToProps? It took me quite a while to figure out what the hell was going on.

We removed it before, and people were unhappy. We do warn in the documentation that we encourage you to follow React flow and avoid connect()ing leaf components.

[esamattis]

Heh, before today I would have been one those unhappy people but after pulling my hair out while debugging this today my opinion is certainly different. Sadly I missed that bit of the docs (or didn't take it seriously at the time ).

[esamattis]

To whom who are looking for a workaround for this: An interesting observation is that if you do the selection in the component render method the issue is not present at all.

Example:

https://github.com/epeli/redux-connect-demo/pull/1/files

The down side is that your component will do more useless re-renders as you'll have to pass in data that is not actually used for rendering.

[gaearon]

Superseded by #99.