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

Fix getDerivedStateFromProps usage #980

Closed
wants to merge 20 commits into from

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Jul 22, 2018

Success!

Using getDerivedStateFromProps properly, this fix passes all existing tests and is a perfect intermediate version prior to react-redux 6.x and 7.x

Fixes #965

breaks 3 tests:

connect.spec.js:
1. React connect should subscribe properly when a middle connected component does not subscribe
2. React connect should pass state consistently to mapState

Provider.spec.js
1. React should pass state consistently to mapState

However, it is possible that the tests fail simply because the ORDER has changed? Will test that theory in a moment
The key is that in gDSFP we grab the current store state instead of using our cached value, and it always works, even with batched redux actions. We also use current props instead of cached ones in relevant portions.
package.json Outdated
@@ -39,15 +39,14 @@
"coverage": "codecov"
},
"peerDependencies": {
"react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0",
"react": "^16.4.0-0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not going to break React compat in this version yet. We need to still support <=16.2, otherwise this is a major revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#965 (comment) Could you and @markerikson decide what you want and let me know?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(point of clarification) Because gDSFP changed in React 16.4, the only way to support both is to have parallel implementations, and a runtime check when connectAdvanced is loaded, something like this pseudo-code:

import React from 'react'

const parts = React.version.split('.')
if (parts[0] + 0 < 16) {
  module.exports = require('old/connectAdvanced')
} else if (parts[0] === '16' && parts[1] + 0 <= 3) {
  module.exports = require('new5.1/connectAdvanced')
} else if (parts[0] === '16' && parts[1] + 0 >= 4 || parts[0] + 0 > 16) {
  module.exports = require('new5.2/connectAdvanced')
}

Or you could use the 5.1 (current test.1 version) with the polyfill for the older React versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gDSFP changed in 16.4 to fire more often (specifically on any update, not just ones from props changes), so it should be compatible with 16.3 and react-lifecycle-compat. If it's not, then this still suffers from a buggy or improper implementation.

One thing that might of use is in #856, which makes use of a setState callback that returns null to cancel the update. I might not take that PR entirely verbatim, but the concept may be helpful to the implementation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because gDSFP has no way of knowing whether an update comes from a state change or from a props change, there is no way to support receiving both props and state (redux changes) that works in both 16.3 and 16.4.

I'm baffled as to why you would think it necessary to explain how the gDSFP API changed as if it wasn't the core of the original bug report this PR and my prior version addresses. Perhaps you're not reading the PR carefully enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You certainly can know and you've implemented such a check. this.state.props is essentially prevProps, which you can check against nextProps to determine the source.

Or, if you wanted to do it differently, you can add something to the setState (which comes through with prevState) to signal that this is a state-based update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, but I need to make a pointed remark. Please don't take it personally, I am merely pointing out an inefficiency in our communication which I would love to see remedied so we can actually solve this problem.

I don't believe you've actually taken any substantive time to look at this pull request before dismissing it, because it literally does everything you have just described. For instance, here is the source code to gDSFP:

      static getDerivedStateFromProps(props, state) {
        if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null
        const nextChildProps = Connect.getChildPropsState(props, state)
        return nextChildProps
      }

Note the comparison of props to state.props.

I misspoke and did not check to see if the patch worked in 16.0-16.3, which would have saved some time as well.

So, I just tried the pull request in React 16.3, and React 16.0 and the latest React 15. All tests except the strict mode test (which only exists in 16.3+) pass in all React 16 versions. In React 15, all the tests fail because the test renderer isn't working to return the root, and that is with the polyfill. I haven't done the research to see why that is happening, but it is an issue with the tests. I also haven't taken the time yet to see if the thing works in a test project even though all the tests fail.

So the main issue is that the peer dependency in package.json is too restrictive, and can be relaxed to include React 16+, and possibly 15, if the issue with the tests can be determined to be only in the tests.

@timdorr
Copy link
Member

timdorr commented Jul 24, 2018

(Moving this up to the top level in case another commit hides our thread above)

No worries. Text-based communication is always lossy.

If the polyfill works in 16.0-16.2 (where it's actually active; it's not in 16.3), then I'd have confidence in it working in older versions of React as well.

As for the PR contents itself, I still have concerns about passing props as a hack around the signature of gDSFP (essentially to make it function like cWRP did before). Something there doesn't smell right. At the very least, there's a bug with this.state.props, as they're never updated past instantiation.

@cellog
Copy link
Contributor Author

cellog commented Jul 24, 2018

You're right, the fact props aren't updated is an issue, and they should be returned from gDSFP, no idea yet if that will affect test failures. I had to be quick this morning, so I may have tested things without polyfill, or with, but it was working no failures 16.0-.4

I'll have time to breathe a bit later, I can check again.

Store last seen props in gDSFP (was an oversight). Does not change behavior, just performance.
@cellog
Copy link
Contributor Author

cellog commented Jul 24, 2018

OK, The last commit relaxes the peer dependency, and adds back the storing of props. It would only affect performance on updates, so didn't affect behavior, which is why tests weren't broken. Take a look now?

Storing props in state would normally be a problem, if we were using it to generate derived props, they could be stale. But it is only used to detect changes in props in order to make derived props generation less expensive, so I don't think we will have a problem storing them this way.

}

static getDerivedStateFromProps(props, state) {
if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add parentheses around the pure && shallowEqual() bit just to clarify the comparison?

}

shouldComponentUpdate(_, nextState) {
return nextState.shouldComponentUpdate
if (!connectOptions['pure']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason for the bracketed field access here? Pretty sure you should be able to dot-access it - that would only be an issue if connectOptions was undefined, which it won't be.

this.notifyNestedSubs()
}
return ret
}, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could change this to notify ? this.notifyNestedSubs : noop .

@@ -217,7 +243,7 @@ export default function connectAdvanced(
}

onStateChange() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all this does is call updateChildPropsFromReduxStore(), we could just pass that to the Subscription instead.

@markerikson
Copy link
Contributor

markerikson commented Jul 25, 2018

I left a few nitpicks stylistically, but code-wise this seems plausible.

I pulled and ran the tests, and it does look like everything's passing. Sweet! I would like to see if there's any differences performance-wise. Let me see if I can round up some of the prior benchmark repos I've seen in the past.

I'd say it's worth merging and publishing as 5.1.0-test.2 or something. Let's get it out where people can try it.

@cellog , thanks a lot for collaborating with us on this! I think we had some nice back-and-forth sharing of ideas here.

Assuming this works, we can start turning our attention to a reworked version of #898 that uses new context.

@cellog
Copy link
Contributor Author

cellog commented Jul 25, 2018

I had the same thought about onStateChange yesterday afternoon when I couldn't get to a machine, and of course forgot about it by evening. I took a moment to fix the nitpicks.

I think before we merge, it is important to add some more tests, specifically to check for how more than 1 update affects its performance and its correctness, since I had forgotten to save props in gDSFP and adding it in didn't affect any tests.

However, I have a larger issue I'd like to bring up relating to tests. Is there any reason the repo doesn't use enzyme? Also, do you think it would be feasible to make the test suite run against multiple React versions? In my gut, I feel uncomfortable publishing when every test fails in 15.x because the suite can't properly run on my machine (the issue is that react-test-renderer API changed dramatically at 16.x). If the tests were switched to use enzyme, then the change is as simple as:

import React from 'react'
import { configure } from 'enzyme'
import Adapter14 from 'enzyme-adapter-react-14'
import Adapter15 from 'enzyme-adapter-react-15'
import Adapter16 from 'enzyme-adapter-react-16'

if (+React.version.split('.')[0] === 0) {
  configure({ adapter: new Adapter14 })
} else if (+React.version.split('.')[0] === 15) {
  configure({ adapter: new Adapter15 })
} else {
  configure({ adapter: new Adapter16 })
}

If there is no blocker to making this change, I'd like to tackle it.

Also, I use wallaby in my IDE to run the tests instantly (even faster than jest, about .75 seconds to run the whole test suite), could I add my config file (it's generic) to the repo for others who might use it? It doesn't affect anyone who doesn't use wallaby, and could be ignored by npm with .npmignore

If you're cool with the test change, I'll open an issue for tracking the switch to enzyme and copy/paste the 2nd portion of this comment

@cellog
Copy link
Contributor Author

cellog commented Jul 25, 2018

P.S. enzyme doesn't support the 16.3 context yet, but it can be easily mocked out

@markerikson
Copy link
Contributor

markerikson commented Jul 25, 2018

I don't know of any specific reason why we've never pulled in Enzyme. And yes, I'd love to see more tests added! We've already seen the benefit of our existing test suite - adding more tests to help clarify expected behavior would be extremely valuable. (It would also be great if we had some comments explaining why some of these current tests exist, but that can be added later.)

I have no idea about the feasibility of targeting multiple React versions with the tests, but it seems like a neat idea if we can do it.

I do find it a bit ironic that the test suite entirely relies on using connect as a decorator, yet we (I?) generally discourage it being used that way.

So yeah, if you're up for it, go ahead and open up a separate PR for improving the test suite, and then we can rebase this branch against that once it's merged in or something.

@timdorr
Copy link
Member

timdorr commented Jul 25, 2018

We haven't pulled in enzyme because we haven't needed it. We would have to rewrite all of our tests to really make use of it. And since the plan is to focus 6.0 on React 16.4+, I don't think we need to worry about targeting multiple React versions. It's a lot of effort that wouldn't have any real advantages.

@cellog
Copy link
Contributor Author

cellog commented Jul 25, 2018

I don't think converting the tests to enzyme will be hard, I did a similar thing in my own project, with 1200 tests. It took me about 1.5 hours, so I don't anticipate this will take much longer :). In any case, it will allow running the current suite against 14 and 15, which currently doesn't happen. I'll make a PR and you can decide if it's worth it.

@markerikson
Copy link
Contributor

Yeah, I see both trains of thought here. On the one hand, 6.0 would presumably obsolete any checks against React <16.5. But, wouldn't hurt to have that around for the 5.x branch. And hey, if @cellog wants to contribute that, I'm not going to argue against free help :)

@cellog
Copy link
Contributor Author

cellog commented Jul 26, 2018

Incidentally, I think we will find this implementation leaks when an error is thrown in mapStateToProps, because if the error boundary simply discards the tree, we will have subscribed in the constructor.

So I'd like to move all the subscription stuff to cDM and cDU. I think it may even be dangerous to do it with a setState callback because we can't know if the component was unmounted in the mean time, since an error won't call lifecycle methods.

I'm not sure how this affects existing tests, but we'll find out.

@timdorr timdorr changed the title Fully fix #965 Fix getDerivedStateFromProps usage Jul 26, 2018
in order to handle subscriptions in state, we have to treat React 16.4 differently from 16.3, because state changes trigger gDSFP in 16.4 and don't in 16.3-
Also improves performance
it seemed componentDidUpdate was needed to ensure subscriptions work, but this is untrue, and causes infinite loops on pre-React 16
// in React 16.3, it doesn't. react-lifecycles-compat matches React 16.3 behavior
// if a future version of react-lifecycles-compat DOES trigger on state changes
// the isOldReact() call must be removed
if (!isOldReact() && state.lastNotify !== state.notifyNestedSubs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to do the string splitting every time this runs. We can just do it once when connect() is called, or even once at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duh. fixing :)


static getChildPropsState(props, state) {
try {
const nextProps = state.childPropsSelector(state.store.getState(), props)
Copy link
Contributor

@markerikson markerikson Aug 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the store state here is still my biggest concern, async-wise, because it seems vulnerable to the "time-slicing" behavior. But, I guess we can go with it until we get more examples from the React team for what proper patterns look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is that if we don't do this, then we will always call mapStateToProps twice, once for props change, once for redux state change, whenever an action is dispatched. This is because when an update occurs, it follows this path:

Parent <- redux
Parent updates state to store new redux state
Parent gDSFP called
Parent props change, 
Parent render called, renders children
Child <- props
child gDSFP called
...
Child <- redux

This is because the parent call to update child subscriptions happens in the getState callback, which is not called until the entire tree has been rendered (at least in the tests). Basically, we can't rely on the subscription happening at a deterministic moment, and so the only way to get a non-stale redux state is to retrieve it just-in-time when a props change comes in.

I don't think this will be a problem, because in the worst case (as I understand it), a calculation might get thrown out and re-started. In other words, I don't think it is possible for a major delay between gDSFP and render, because if rendering is suspended, and redux state changes, we will get an async event that calls setState, which will re-trigger gDSFP when rendering resumes.

In addition, if props change after a delay, gDSFP will have to be called again prior to rendering. The worst case is that the stored redux state is stale, which is OK, because if an action is dispatched, we will compare the stale state to the actual state, and determine that we should re-run mSP to see if derived props have changed. So we won't ever be incorrect, just possibly slightly inefficient. But since we are talking about a single extra call to mSP over the course of many seconds, it should not be significant for all but the most unusual cases where the redux state is updating constantly and a large number of components are rendering in response to changes.

So I put this in because in my reasoning, it saves us from constantly calling mSP unnecessarily twice as often, at the expense of occasionally calling it twice after a long delay due to suspense (as in seconds), where performance will not be a noticeable issue.

The next thing is to start playing with the suspense demo and see if we can get it to break the expectations I laid out above

if (this.subscription) {
oldListeners = this.subscription.listeners.get()
this.subscription.tryUnsubscribe()
if (this.state.subscription) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't wait to make all this stuff hopefully go away in 6.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully? the new Context API is a subscription! It's going to cut hundreds of LOC and improve performance dramatically. That's my prediction

if (state.subscription.isReady() && !hotReloadCallback) return null
// parentSub's source should match where store came from: props vs. context. A component
// connected to the store via props shouldn't use subscription from context, or vice versa.
this.state.subscription.hydrate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting nitpick: can we spread the lines in this method out some? Seems kinda crammed together, and the empty line after return is odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// To handle the case where a child component may have triggered a state change by
// dispatching an action in its componentWillMount, we have to re-run the select and maybe
// re-render.
this.updateChildPropsFromReduxStore(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very similar to the extra check in Brian's createSubscription() util.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that comment was legacy from the old subscription code, and doesn't really apply to the next line any more, so I removed it

@markerikson
Copy link
Contributor

markerikson commented Aug 4, 2018

So, we appear to have a PR that passes all existing tests as-is, other than the tiny tweak to where we're checking for a store instance.

This may at least be ready for release as a 5.1.0.test-2, other than the small nitpicks I just listed out.

I'm going to see if I can start putting together a perf benchmark harness, and I'd like to see how this PR compares to both 4.4.9 and 5.0.7.

@cellog
Copy link
Contributor Author

cellog commented Aug 4, 2018

P.S. I do have one concern. Right now, subscription contains parentSub at creation time, and I think this counts as a side effect. However, I think the best path going forward is to get 6.x out as soon as possible, which will remove any possible side effect subscription issues. The subscription to redux is handled properly since we use setState to trigger re-renders on new actions.

@markerikson
Copy link
Contributor

I don't think that's too much of a concern. If a component is created and then thrown away before mounting, its subscription would ultimately get GC'd, and it hasn't produced any lasting changes outside of itself (as far as I understand things atm). But yes, totally agree that we ought to start aggressively working on 6.0 ASAP.

@markerikson
Copy link
Contributor

After discussion with both @cellog and @timdorr , we're leaning towards not putting out a 5.1 release. Trying to manage lifecycle behavior across multiple React versions is difficult (and apparently the react-lifecycles-compat polyfill uses the 16.3 behavior, not 16.4 , which doesn't help).

I'm currently trying to work on putting together a performance benchmark harness so that we can start comparing comparing behavior between different releases. From there, I plan to pick up work on #898 , or at least a variation of that approach, and start serious development on a 6.0 release that would probably be 16.5+ only.

@cellog
Copy link
Contributor Author

cellog commented Aug 14, 2018

I think this PR is fairly obsolete, closing

@cellog cellog closed this Aug 14, 2018
@deecewan
Copy link

@markerikson sorry to revive an old thread, but never putting out a 5.1.0 release? I've been hanging out for this release (mostly because of the warning about gDSFP and cWU being used at the same time).

Is the test release stable enough for use?

@cellog
Copy link
Contributor Author

cellog commented Aug 15, 2018

@deecewan the problem we encountered is that fixing the issue in gDSFP is incredibly complex, and results in a build about 4 times slower than 5.0.7 when benchmarked. If you stick to 16.3 or lower, the 5.1.0-test.1 will be stable, but it fails spectacularly on React 16.4

@timdorr
Copy link
Member

timdorr commented Aug 15, 2018

@deecewan We're doing a 6.0 release that will target 16.4+ (technically 16.3 too, but I consider that a broken release), so that's essentially what 5.1.0 will become.

@deecewan
Copy link

@timdorr is there an issue list for the 6.0 release? anything I can help out with?

@cellog
Copy link
Contributor Author

cellog commented Aug 15, 2018

@deecewan #995 #1000 but especially #983 and the discussion in #890 is also relevant

@markerikson
Copy link
Contributor

@deecewan : the 5.1.0-test.1 release is completely broken - don't use it. This PR was attempting to fix it.

Just to check , what version of React are you on currently? Are you actively adding <StrictMode> tags to your app?

@markerikson
Copy link
Contributor

@deecewan : we don't have a formal issues list for 6.0 yet - we probably ought to start coming up with one.

As @cellog said, right now we've got two different PRs that try to rework connect to use React.createContext(), with varying approaches. I'm at React Rally this week, and hope to talk to Brian Vaughn and get his feedback on the two implementations. From there we can hopefully get a better picture of what 6.0 needs to include.

I've also been trying to put together a perf benchmark harness over in #983 . I'd certainly appreciate some help trying to flesh that out, and officially add it to the React-Redux repo.

@deecewan
Copy link

I'm on 16.4.1. No StrictMode yet - but I still get warnings in the console that I'm using both gDSFP and cWU (cWU coming from react-redux).

@cellog
Copy link
Contributor Author

cellog commented Aug 16, 2018

@deecewan that seems unlikely to be a result of using react-redux 5.0, since the warning from react comes from a single class definition, and react-redux 5.0 connectAdvanced does not define gDSFP. Can you elaborate on the warning (or paste a copy) to help understand what's causing it? Is it this warning?

facebook/react#12419

@markerikson
Copy link
Contributor

@deecewan : our end goal is that if you're on React 16.4 and React-Redux 5, you should hopefully be able to do a near drop-in upgrade when we finally have v6 ready. There will be at least a few API differences, but hopefully they'll be small (things like replacing the withRef option with use of React's forwarded refs, etc).

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.

[5.1.0-test.1] Connected components don't re render after state update
6 participants