Convert Provider into function component with hooks #1377
Conversation
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
| subscription.onStateChange = this.notifySubscribers | ||
|
|
||
| this.state = { | ||
| subscription.onStateChange = subscription.notifyNestedSubs |
There was a problem hiding this comment.
This is what I determined subscription.onStateChange = this.notifySubscribers to actually boiled down to. I'm wondering if subscription couldn't just handle this internally?
There was a problem hiding this comment.
We use the same Subscription class for the connect components, where this is part of our top-down update mechanic. So, it needs to stay configurable. This is just a quirk of re-use.
There was a problem hiding this comment.
I know this is a very old PR, but I'm wondering if you ever tested this in StrictMode? I'm noticing bugs where updates stop happening for us using react-redux 7.2.5, and I believe it's because we set onStateChange here in the useMemo callback, but then we clear it here in the useEffect teardown. With strict mode, the useMemo callback here will run twice, but the result of the second run isn't actually used. So, the result is that children are rendered with the original subscription which is resubscribed but has no onStateChange callback, so it does nothing. I think the fix is to move this line to the useEffect call. I can patch that for us locally, but I'm wondering if you ever encountered this after making this change.
There was a problem hiding this comment.
@MichaelSnowden yeah, we actually fixed that bug in https://github.com/reduxjs/react-redux/releases/tag/v7.2.8 :) latest 7.x release is 7.2.9. (But also, we would recommend upgrading to v8.x if possible if you're still on React 17 or earlier, or v9 if you're on React 18.)
There was a problem hiding this comment.
Thanks @markerikson ! Do you know if it's possible for this to occur outside of StrictMode with React 18? I don't think we'll be able to upgrade unfortunately, but we can patch this fix. The main thing I'm wondering is if this could explain other times I've heard devs say that the Redux store just stopped updating their components (even outside of strict mode).
I don't understand React concurrent mode fully, but it seems like it might be possible there for something simillar to happen there where the subscription useMemo state is reused, but this effect runs twice (because the component remounted but kept its state somehow). Do you know if that's possible or if this is a bug we could have only seen in strict mode.
There was a problem hiding this comment.
Strict Mode would be the only real reason I can think of that this would be an issue off the top of my head.
|
Deploy preview for react-redux-docs ready! Built with commit 67e873d |
| @@ -312,5 +312,43 @@ describe('React', () => { | |||
| ReactDOM.unmountComponentAtNode(div) | |||
| expect(spy).toHaveBeenCalledTimes(1) | |||
| }) | |||
|
|
|||
| it('should handle store and children change in a the same render', () => { | |||
There was a problem hiding this comment.
I did verify that this did actually fail on the old code before making my change.
| } | ||
|
|
||
| componentWillUnmount() { | ||
| if (this.unsubscribe) this.unsubscribe() |
There was a problem hiding this comment.
I could not see where this.unsubscribe was actually being set. I think this could have been safely removed, but the new implementation does not allow for this in any way, so if that is an issue I'll have to go back to the drawing board.
There was a problem hiding this comment.
Yeah, I think this line is a leftover artifact from pre-7.0 that I missed removing.
|
Hey, thanks for putting this together! I'll have to come back and look at it a bit later, but noted. |
|
hey @markerikson / @timdorr, have either of you had a chance to take a look at this yet? |
|
Looks good to me. @markerikson? |
Co-Authored-By: Mark Erikson <[email protected]>
|
Okay, board is green. LGTM! |
|
Hey, just curious what the release timeframe is on this. Is there a specific milestone this will wait for or will a |
|
Good question. @timdorr , you got time to drop a new release with this and the memory clear tweak? I'm at React Rally this week. Busy with the conf the next two days. Hoping to do some maintainer-ish stuff on the weekend. If it's not out by then, I'll try to publish it. |
|
No major rush from me, just curious. |
|
This is out as 7.1.1 now! |
+ 7.1.1 and above cause widespread test failures as a result of them changing to the use of a function component / hooks: reduxjs/react-redux#1377
+ 7.1.1 and above cause widespread test failures as a result of them changing to the use of a function component / hooks: reduxjs/react-redux#1377
* 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]>
Fixes issue where the
storeandchildrenvalues were updated in different render passes which could result inmapStateerrors if the new children were not compatible with the old store.The change to a function component meant
Providercould take advantage ofuseEffectto "watch" thestoreprop and unsubscribe to the old store and subscribe to the new store.Fixes #1370