Fix getDerivedStateFromProps usage #980
Conversation
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", | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
#965 (comment) Could you and @markerikson decide what you want and let me know?
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
(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 |
|
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.
|
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. |
src/components/connectAdvanced.js
Outdated
| } | ||
|
|
||
| static getDerivedStateFromProps(props, state) { | ||
| if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null |
There was a problem hiding this comment.
Maybe add parentheses around the pure && shallowEqual() bit just to clarify the comparison?
src/components/connectAdvanced.js
Outdated
| } | ||
|
|
||
| shouldComponentUpdate(_, nextState) { | ||
| return nextState.shouldComponentUpdate | ||
| if (!connectOptions['pure']) { |
There was a problem hiding this comment.
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.
src/components/connectAdvanced.js
Outdated
| this.notifyNestedSubs() | ||
| } | ||
| return ret | ||
| }, () => { |
There was a problem hiding this comment.
Could change this to notify ? this.notifyNestedSubs : noop .
src/components/connectAdvanced.js
Outdated
| @@ -217,7 +243,7 @@ export default function connectAdvanced( | |||
| } | |||
|
|
|||
| onStateChange() { | |||
There was a problem hiding this comment.
If all this does is call updateChildPropsFromReduxStore(), we could just pass that to the Subscription instead.
|
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 @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. |
|
I had the same thought about 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 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 |
|
P.S. enzyme doesn't support the 16.3 context yet, but it can be easily mocked out |
|
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 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. |
|
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. |
|
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. |
|
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 :) |
|
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. |
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
src/components/connectAdvanced.js
Outdated
| // 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) { |
There was a problem hiding this comment.
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.
|
|
||
| static getChildPropsState(props, state) { | ||
| try { | ||
| const nextProps = state.childPropsSelector(state.store.getState(), props) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <- reduxThis 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) { |
There was a problem hiding this comment.
I can't wait to make all this stuff hopefully go away in 6.0.
There was a problem hiding this comment.
hopefully? the new Context API is a subscription! It's going to cut hundreds of LOC and improve performance dramatically. That's my prediction
src/components/connectAdvanced.js
Outdated
| 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() |
There was a problem hiding this comment.
Formatting nitpick: can we spread the lines in this method out some? Seems kinda crammed together, and the empty line after return is odd.
src/components/connectAdvanced.js
Outdated
| // 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) |
There was a problem hiding this comment.
This seems very similar to the extra check in Brian's createSubscription() util.
There was a problem hiding this comment.
that comment was legacy from the old subscription code, and doesn't really apply to the next line any more, so I removed it
|
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 This may at least be ready for release as a 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. |
|
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. |
|
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. |
|
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 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. |
|
I think this PR is fairly obsolete, closing |
|
@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 Is the |
|
@deecewan the problem we encountered is that fixing the issue in |
|
@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. |
|
@timdorr is there an issue list for the 6.0 release? anything I can help out with? |
|
@deecewan : the Just to check , what version of React are you on currently? Are you actively adding |
|
@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 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. |
|
I'm on 16.4.1. No |
|
@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? |
|
@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 |
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