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

Provider uses previous store for one additional render when store changes #1370

Closed
mpeyper opened this issue Aug 2, 2019 · 2 comments · Fixed by #1377
Closed

Provider uses previous store for one additional render when store changes #1370

mpeyper opened this issue Aug 2, 2019 · 2 comments · Fixed by #1377

Comments

@mpeyper
Copy link
Contributor

mpeyper commented Aug 2, 2019

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

If a the store passed to a Provider changes the store in ReactReduxContext does not update until the next render. This is causing me an issue the children are changing, causing the to crash when trying to mapStateToProps in that additional render.

Sandbox.

Note: this is a contrived example to to show how this occurs, but I have an real world case using React Router V5 that has the same issue.

Also Note: my use case is not to have a distinct store per page, but actually using redux-subspace to create a virtual sub-store per page. Again, I didn't bother with this detail for the example as it occurs with real stores too.

What is the expected behavior?

I would expect that if Provider permits me to change the store, that a connected component below it would not crash when rendering. I believe the root of the issue is the fact that the store is not updated in the Providers's state until componentDidUpdate, but the render the would result in this update already has the new this.props.children value.

I do concede that this is a pretty niche use case, and may not be the highest priority to fix (assuming you agree that it is an issue at all).

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

"react": "16.8.6",
"react-dom": "16.8.6",
"redux": "4.0.4"
"react-redux": "7.1.0",
@mpeyper
Copy link
Contributor Author

mpeyper commented Aug 2, 2019

Workarounds I have right now:

  1. key the Provider in some way (not actually practical in some cases)
  2. use ReactReduxContext to pluck the store out context and compare it to the store passed to Provider and if it is not equal, render null (works, but a bit.... smelly)
  3. rethink my life choices

@markerikson
Copy link
Contributor

If you can rewrite Provider to fix the behavior, please go ahead and file a PR.

mpeyper added a commit to mpeyper/react-redux that referenced this issue Aug 11, 2019
Fixes issue where the store and children got updated in different render passes which could result in mapState errors if the new children were not compatible with the old store.
Fixes reduxjs#1370
markerikson added a commit that referenced this issue Aug 20, 2019
* Convert Provider into function component with hooks

Fixes issue where the store and children got updated in different render passes which could result in mapState errors if the new children were not compatible with the old store.
Fixes #1370

* Remove onStateChange from subscription when unsubscribing

Co-Authored-By: Mark Erikson <[email protected]>
albertodev7 pushed a commit to albertodev7/react-redux that referenced this issue Dec 8, 2022
* Convert Provider into function component with hooks

Fixes issue where the store and children got updated in different render passes which could result in mapState errors if the new children were not compatible with the old store.
Fixes reduxjs#1370

* Remove onStateChange from subscription when unsubscribing

Co-Authored-By: Mark Erikson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants