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

Persist Nested Subscriptions Through Hot Reload #715

Merged
merged 1 commit into from
Jul 27, 2017
Merged

Persist Nested Subscriptions Through Hot Reload #715

merged 1 commit into from
Jul 27, 2017

Conversation

dsgkirkby
Copy link
Contributor

This resolves #670.
This likely also resolves #636 but I encounter errors installing the linked repo on my machine.

Cause Of Issue

When a connected component hot reloads, it unsubscribes and then resubscribes to store updates. As part of unsubscription, all child listeners (also referred to as nestedSubs) are removed. For child components that also hot-reload, this is fine, as they will resubscribe during the hot reload. However, if any of the hot reloading parent's descendants are connected but do not hot reload, they are never resubscribed to store updates.

Solution

To resolve this, we persist the previous component's listeners to the reloaded version of the component, so that descendants that did not reload are still subscribed properly to store updates.

Downsides

This does mean that the old versions of hot reloaded components are still subscribed, but when they unmount as part of the reload, their selector.shouldComponentUpdate, selector.run, and notifyNestedSubs are all set to no-ops, making their onStateChange always a no-op itself.

@markerikson
Copy link
Contributor

Sweet, thanks! We'll definitely want @jimbolla to take a look at this, and I'll try to find time to look over it myself as well.

@jimbolla
Copy link
Contributor

jimbolla commented Jun 8, 2017

At first glance it makes sense, but I'll have to think awhile about any foreseeable issues.

@jimbolla
Copy link
Contributor

jimbolla commented Jun 8, 2017

At any rate, any issues would only affect hot-reloading, which is currently broken, so this is almost certainly an improvement.

@GeKorm
Copy link

GeKorm commented Jul 27, 2017

Is something blocking this? Is there anything we can do to help?

@adkasp
Copy link

adkasp commented Jul 27, 2017

it would make a life much easier for many devs, please pull

@jimbolla
Copy link
Contributor

I think this is OK to merge.

@timdorr
Copy link
Member

timdorr commented Jul 27, 2017

Cool. Here we go...

@timdorr timdorr merged commit a5e45d9 into reduxjs:master Jul 27, 2017
@dsgkirkby
Copy link
Contributor Author

Thanks guys!

@3upzorz
Copy link

3upzorz commented Aug 8, 2017

Any idea when this will be published?

@olegstepura
Copy link

Hi!

This PR should fix an issue #636 which makes our work very unconfortable due to hot reload issues. Personally I tried to downgrade to latest v4 as a workaround, but this broke rounting with react-router v4, so it was not an option for me.

I think I will speak for many developers waiting if I'll kindly ask you to please release this fix as soon as possible.

Thaтks a lot!

@timdorr
Copy link
Member

timdorr commented Aug 8, 2017

OK, pushed out 5.0.6.

@pugnascotia
Copy link

I ran into this just the other day, so awesome to get it fixed! Thanks everyone!

@olegstepura
Copy link

This seem to fix #636

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.

HMR: connected component gets disconnected from store react-redux 5.0.3 breaks hot-reloading
9 participants