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

Convert Provider into function component with hooks #1377

Merged
merged 2 commits into from Aug 20, 2019

Conversation

@mpeyper
Copy link
Contributor

mpeyper commented Aug 11, 2019

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

The change to a function component meant Provider could take advantage of useEffect to "watch" the store prop and unsubscribe to the old store and subscribe to the new store.

Fixes #1370

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
subscription.onStateChange = this.notifySubscribers

this.state = {
subscription.onStateChange = subscription.notifyNestedSubs

This comment has been minimized.

Copy link
@mpeyper

mpeyper Aug 11, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@timdorr

timdorr Aug 12, 2019

Member

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.

@netlify

This comment has been minimized.

Copy link

netlify bot commented Aug 11, 2019

Deploy preview for react-redux-docs ready!

Built with commit 67e873d

https://deploy-preview-1377--react-redux-docs.netlify.com

@@ -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', () => {

This comment has been minimized.

Copy link
@mpeyper

mpeyper Aug 11, 2019

Author Contributor

I did verify that this did actually fail on the old code before making my change.

}

componentWillUnmount() {
if (this.unsubscribe) this.unsubscribe()

This comment has been minimized.

Copy link
@mpeyper

mpeyper Aug 11, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@markerikson

markerikson Aug 11, 2019

Contributor

Yeah, I think this line is a leftover artifact from pre-7.0 that I missed removing.

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 11, 2019

Hey, thanks for putting this together! I'll have to come back and look at it a bit later, but noted.

@mpeyper

This comment has been minimized.

Copy link
Contributor Author

mpeyper commented Aug 19, 2019

hey @markerikson / @timdorr, have either of you had a chance to take a look at this yet?

@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Aug 19, 2019

Looks good to me. @markerikson?

Co-Authored-By: Mark Erikson <mark@isquaredsoftware.com>
@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 20, 2019

Okay, board is green. LGTM!

@markerikson markerikson merged commit 0c5f764 into reduxjs:master Aug 20, 2019
7 checks passed
7 checks passed
Header rules - react-redux-docs No header rules processed
Details
Pages changed - react-redux-docs All files already uploaded
Details
Mixed content - react-redux-docs No mixed content detected
Details
Redirect rules - react-redux-docs 5 redirect rules processed
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +1.63% compared to 4b9cece
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/react-redux-docs/deploy-preview Deploy preview ready!
Details
@mpeyper

This comment has been minimized.

Copy link
Contributor Author

mpeyper commented Aug 22, 2019

Hey, just curious what the release timeframe is on this. Is there a specific milestone this will wait for or will a 7.1.1 version get pushed out soon?

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Aug 22, 2019

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.

@mpeyper

This comment has been minimized.

Copy link
Contributor Author

mpeyper commented Aug 22, 2019

No major rush from me, just curious.

@mpeyper mpeyper deleted the mpeyper:fix-1370 branch Aug 22, 2019
@timdorr

This comment has been minimized.

Copy link
Member

timdorr commented Aug 26, 2019

This is out as 7.1.1 now!

aaronlademann-wf added a commit to cleandart/react-dart that referenced this pull request Dec 11, 2019
+ 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
@kealjones-wk kealjones-wk mentioned this pull request Dec 12, 2019
0 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.