Fix getDerivedStateFromProps usage #980

cellog · 2018-07-22T20:23:21Z

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.

timdorr · 2018-07-23T00:51:45Z

package.json

@@ -39,15 +39,14 @@
    "coverage": "codecov"
  },
  "peerDependencies": {
-    "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0",
+    "react": "^16.4.0-0",


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.

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

(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.

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.

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?

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.

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 · 2018-07-24T14:18:29Z

(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 · 2018-07-24T18:14:57Z

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 · 2018-07-24T21:53:28Z

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.

markerikson · 2018-07-25T06:50:11Z

src/components/connectAdvanced.js

+      }
+
+      static getDerivedStateFromProps(props, state) {
+        if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null


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

markerikson · 2018-07-25T06:51:17Z

src/components/connectAdvanced.js

      }

      shouldComponentUpdate(_, nextState) {
-        return nextState.shouldComponentUpdate
+        if (!connectOptions['pure']) {


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.

markerikson · 2018-07-25T06:54:00Z

src/components/connectAdvanced.js

+            this.notifyNestedSubs()
+          }
+          return ret
+        }, () => {


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

markerikson · 2018-07-25T06:54:52Z

src/components/connectAdvanced.js

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

      onStateChange() {


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

markerikson · 2018-07-25T07:01:29Z

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 · 2018-07-25T10:53:57Z

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 · 2018-07-25T10:56:27Z

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

markerikson · 2018-07-25T13:01:56Z

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 · 2018-07-25T15:39:32Z

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 · 2018-07-25T15:41:42Z

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 · 2018-07-25T16:22:25Z

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 · 2018-07-26T02:06:42Z

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

markerikson · 2018-08-03T23:52:11Z

src/components/connectAdvanced.js

+        // 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) {


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.

duh. fixing :)

markerikson · 2018-08-04T00:13:07Z

src/components/connectAdvanced.js

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


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.

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

markerikson · 2018-08-04T00:15:27Z

src/components/connectAdvanced.js

-          if (this.subscription) {
-            oldListeners = this.subscription.listeners.get()
-            this.subscription.tryUnsubscribe()
+          if (this.state.subscription) {


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

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

markerikson · 2018-08-04T00:18:36Z

src/components/connectAdvanced.js

+          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()


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

markerikson · 2018-08-04T00:19:55Z

src/components/connectAdvanced.js

+          // 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)


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

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 · 2018-08-04T00:40:08Z

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 · 2018-08-04T02:29:23Z

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 · 2018-08-04T02:41:51Z

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 · 2018-08-07T03:48:06Z

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 · 2018-08-14T17:22:27Z

I think this PR is fairly obsolete, closing

deecewan · 2018-08-15T07:22:03Z

@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 · 2018-08-15T13:10:58Z

@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 · 2018-08-15T13:47:09Z

@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 · 2018-08-15T13:51:43Z

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

cellog · 2018-08-15T14:12:07Z

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

markerikson · 2018-08-15T20:24:36Z

@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 · 2018-08-15T20:29:35Z

@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 · 2018-08-16T02:58:31Z

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 · 2018-08-16T04:02:53Z

@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 · 2018-08-16T04:10:50Z

@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).

cellog added 5 commits July 20, 2018 20:24

up min react versions

b631441

typo

de4e4f3

success! gDSFP-based react-redux

f751ac9

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.

up the peer dependency to React 16.4

9284894

cellog mentioned this pull request Jul 22, 2018

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

Closed

remove debugging code

948c20f

timdorr reviewed Jul 23, 2018

View reviewed changes

cellog mentioned this pull request Jul 23, 2018

fix connected props derived props generation #975

Closed

use polyfill, make peer dependency relaxed

490b0e3

Store last seen props in gDSFP (was an oversight). Does not change behavior, just performance.

markerikson reviewed Jul 25, 2018

View reviewed changes

fix style details

86e986b

markerikson mentioned this pull request Jul 25, 2018

Add performance benchmark suite #983

Closed

markerikson mentioned this pull request Jul 25, 2018

[Umbrella] Releasing Suspense facebook/react#13206

Open

63 tasks

cellog mentioned this pull request Jul 26, 2018

Implement testing in React 0.14, 15, 16, add wallaby.js, use enzyme #984

Merged

timdorr changed the title ~~Fully fix #965~~ Fix getDerivedStateFromProps usage Jul 26, 2018

cellog added 5 commits August 2, 2018 22:41

Merge react-redux master into another-fix-5.1

d50fb7d

remove console statements

1d8ab34

fix for React 16-16.3

fe60a4b

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-

fix breaking tests

72fca1c

Also improves performance it seemed componentDidUpdate was needed to ensure subscriptions work, but this is untrue, and causes infinite loops on pre-React 16

remove unnecessary Promise (React lifecycle does this for us)

1285e4e

markerikson reviewed Aug 3, 2018

View reviewed changes

markerikson reviewed Aug 4, 2018

View reviewed changes

fix nitpicks and use a constant instead of function call every connect()

35e9d12

This was referenced Aug 7, 2018

Consolidated React 16.3+ compatibility and roadmap discussion #950

Closed

[Experimental] API for reading context from within any render phase function facebook/react#13139

Merged

Use React.createContext() by @markerikson #995

Closed

cellog closed this Aug 14, 2018

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

		onStateChange() {

Fix getDerivedStateFromProps usage #980

Fix getDerivedStateFromProps usage #980

Conversation

cellog commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdorr commented Jul 24, 2018

cellog commented Jul 24, 2018

cellog commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Jul 25, 2018 • edited Loading

cellog commented Jul 25, 2018

cellog commented Jul 25, 2018

markerikson commented Jul 25, 2018 • edited Loading

timdorr commented Jul 25, 2018

cellog commented Jul 25, 2018

markerikson commented Jul 25, 2018

cellog commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markerikson commented Aug 4, 2018 • edited Loading

cellog commented Aug 4, 2018

markerikson commented Aug 4, 2018

markerikson commented Aug 7, 2018

cellog commented Aug 14, 2018

deecewan commented Aug 15, 2018

cellog commented Aug 15, 2018

timdorr commented Aug 15, 2018

deecewan commented Aug 15, 2018

cellog commented Aug 15, 2018

markerikson commented Aug 15, 2018

markerikson commented Aug 15, 2018

deecewan commented Aug 16, 2018

cellog commented Aug 16, 2018

markerikson commented Aug 16, 2018

markerikson commented Jul 25, 2018 •

edited

Loading

markerikson commented Jul 25, 2018 •

edited

Loading

markerikson Aug 4, 2018 •

edited

Loading

markerikson commented Aug 4, 2018 •

edited

Loading