Skip to content

Fixes subscription bug when a new store is passed as a prop. #589

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

Merged
merged 3 commits into from
Feb 6, 2017
Merged

Fixes subscription bug when a new store is passed as a prop. #589

merged 3 commits into from
Feb 6, 2017

Conversation

jimbolla
Copy link
Contributor

@davidkpiano
Copy link

Thanks @jimbolla - the general idea is that this should work:

// overly simplified for clarity

<Provider store={store}>
  <Foo>
    <Bar store={ownStore} />
  </Foo>
</Provider>

If I'm not mistaken, <Bar> should subscribe to its ownStore as if it were its own <Provider> (because it essentially is) instead of depending on parent subscriptions.

@jimbolla
Copy link
Contributor Author

Correct. I think there's an edge case to this edge case I'm not covering so I'm gonna keep working on it, but this should be better.

@jimbolla jimbolla changed the title Fixes subscription bug when a new store is passed as a prop. [DO NOT MERGE] Fixes subscription bug when a new store is passed as a prop. Dec 28, 2016
@jimbolla jimbolla changed the title [DO NOT MERGE] Fixes subscription bug when a new store is passed as a prop. Fixes subscription bug when a new store is passed as a prop. Dec 28, 2016
@jimbolla
Copy link
Contributor Author

OK. Now I think I covered the edge cases correctly.

@jimbolla
Copy link
Contributor Author

@timdorr Thoughts on pushing a patch release for this and the other handful of fixes we have waiting to go out?

@davidkpiano
Copy link

Any updates on this? Will it be able to be merged in soon? 🙏

@markerikson
Copy link
Contributor

@jimbolla , @timdorr : following up on this at @davidkpiano 's request. It's Jim's code thus far, and Tim's been cutting releases (something I should go figure out for myself at some point, but don't have time to dig into atm). Are we good to go on this?

@timdorr
Copy link
Member

timdorr commented Jan 31, 2017

I haven't had time to really dig into this one. It's a non-trivial change, hence the hesitation to just blindly merge it in. If anyone else wants to review, I trust them to point out any problems.

@markerikson
Copy link
Contributor

Yeah, that's actually how I feel about this myself at the moment :)

@jimbolla
Copy link
Contributor Author

jimbolla commented Feb 5, 2017

@timdorr @markerikson It might be easier to review the 2 commits separately. The bugfix one is pretty small and the refactor one just moves a few things and adds some more comments.

@timdorr
Copy link
Member

timdorr commented Feb 6, 2017

OK, LGTM. In the future, I'd submit refactors as either separate PRs or just directly onto master.

@timdorr timdorr merged commit e548778 into reduxjs:master Feb 6, 2017
@timdorr timdorr mentioned this pull request Feb 10, 2017
@anru
Copy link

anru commented Feb 14, 2017

@timdorr please release this fix

@BerndWessels
Copy link

BerndWessels commented Feb 28, 2017

@timdorr The react-redux 5.0.3 update breaks HMR with latest react-hot-loader 3.0.0-beta.6.

Basically after a hot-reload the state is somehow broken.

This worked fine in react-redux 5.0.2 :(

repo to reproduce: https://github.com/BerndWessels/react-redux-rr4-wp2-rhl3.git

Work fine with react-redux 5.0.2 but broken when you upgrade to react-redux 5.0.3

@jimbolla jimbolla deleted the fixes-store-as-prop-bug branch March 21, 2017 14:30
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
…#589)

* Fixes subscription bug when a new store is passed as a prop.

Reported here: davidkpiano/react-redux-form#592

* Refactors and adds more comments to connectAdvanced
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 this pull request may close these issues.

6 participants