fix connected props derived props generation

[cellog]

This commit fixes #965

The essence of the problem is that getDerivedStateFromProps is called when the incoming props OR incoming local state changes. So we cannot store anything in state that is needed in shouldComponentUpdate.

This commit splits up the tracking of incoming props, incoming store state changes, and removes getDerivedStateFromProps and the usage of local state to store any information.

Instead, local state is used as a flag solely to track whether the incoming store state has changed.

Since derived props are needed in shouldComponentUpdate, it is generated there and then compared to the previous version of derived props.

If forceUpdate() is called, this bypasses sCU, and so a check in render() compares the props that should have been passed to sCU to those passed to render(). If they are different, it generates them just-in-time.

To summarize:

1. shouldComponentUpdate is ONLY used to process changes to incoming props
2. runUpdater (renamed to triggerUpdateOnStoreStateChange) checks to see if the store state has changed, and stores the state, then updates the counter in local state in order to trigger a new sCU call to re-generate derived state.

Because of these changes, getDerivedStateFromProps and the polyfill are both removed.

All tests pass on my machine, but there is at least 1 side effects to the new design:

• Many of the tests pass state unchanged to props, and pass this to child components. With these changes, the two updates are processed separately. Props changes are processed first, and then state changes are processed.

I updated the affected tests to show that there are "in-between" states where the state and props are inconsistent and mapStateToProps is called with these changes. If the old behavior is desired, that would require another redesign, I suspect.

[markerikson]

Skimming this very briefly. My first reaction is that I'm pretty sure we're supposed to be avoiding keeping stuff on the component instance in order for async/Suspense compatibility to work going forward. I'll try to look at it in more detail in the near future.

[cellog]

If we can't put any data on the component instance, then we would probably need to set up a component registry, and use a unique key in component state to fetch the current stuff. Starts to feel hacky. I hope you're wrong! Have a nice vacation, I'll try to read about the async and suspense stuff and if there is an obvious no-no, I'll see if I can update the PR to fix it.

[markerikson]

The impression I've gotten is that for things to be properly async-safe, both "data" variables and "non-data" variables should both be kept in component state. React keeps props and state internally consistent between re-renders, per facebook/react#11527 .

Rough example, based on what I know. Say React starts to re-render a connected component, and we run some logic that updates an instance field on the component. However, React pauses the update process partway through calculating the diff for the whole tree, then another update happens (like a keystroke) during that pause. React sets aside the WIP diff, calculates a new component tree state from the keystroke, applies it, then goes back to look at the previous update that got interrupted. Depending on where that logic lived, it could get run again, except that if it's basing its decision off an instance field, that field got bumped without the end result actually being applied to the component tree. So, when our update logic runs again, it could be doing so against incorrect information.

That's why gDSFP is static, to avoid users referring to this and instance fields - in order to be safe, decisions need to be made just based on a properly matched pair of component state and component props. It does seem that can lead to a lot of awkwardness as you start to shoehorn other variables into component state, especially since React devs have always been told / assumed that component state was "only for values that affect re-rendering". On the other hand, ReasonReact does indeed use this approach of putting other values (including refs) into component state, so there's precedence.

I'm honestly not sure how this is going to affect our overall memoization approach. v5 (and to a lesser extent v4) relies on internal memoization for all the handling of mapState and mapDispatch, and I can imagine the sequence I described causing the memoization to not handle things correctly.

[cellog]

The key to fixing redux is understanding what it means to "continue an update." What I've seen looks like the lifecycle will restart, so we could see multiple calls to pre-render methods, and multiple calls to render(). The main thing about redux is that we aren't deriving local state from props alone. We derive it from props and an external state mechanism that is outside of React. Thus it is logically impossible to derive state from props, and that is why 5.1.0-test-1 breaks in React 16.4. The implementation uses closures to simulate an instance method (getting store inside gDSFP).

In other words, if the user is going to load something to populate redux state, and wants to take advantage of the new React stuff, instead of using middleware, they will want to write a fetcher that dispatches an action on completion. Then, when the state updates, it will trigger a local state update that reloads the component.

I could see this as providing a ReduxPromise that can be used in a fetcher:

new store.ReduxPromise(loader, result => actions.loadedStuff(result))

(or some such similar API)

But I don't see how there is any way the async stuff will mess up the PR, unless there is some weird path by which shouldComponentUpdate is called, and then state is changed, and render() is called without re-calling sCU, which would pretty much break everything, wouldn't it?

[timdorr]

You shouldn't be doing work like this in sCU. This is going to incur the cost of computing the derived props even if the source of render isn't from the store. Performance will be significantly worse that the current implementation.

[markerikson]

I'm not sure the perf is actually worse. v5 already feeds incoming props into its giant-sized selector in cWRP, which runs before sCU, and the selector appears to bail out early if ownProps haven't changed:

react-redux/src/connect/selectorFactory.js

Lines 74 to 84 in 3e53ff9

	function handleSubsequentCalls(nextState, nextOwnProps) {
	const propsChanged = !areOwnPropsEqual(nextOwnProps, ownProps)
	const stateChanged = !areStatesEqual(nextState, state)
	state = nextState
	ownProps = nextOwnProps
	if (propsChanged && stateChanged) return handleNewPropsAndNewState()
	if (propsChanged) return handleNewProps()
	if (stateChanged) return handleNewState()
	return mergedProps
	}

So, in the case where we're running because the parent re-rendered, I think this just boils down to a standard shallow equality check on ownProps.

[timdorr]

I'd also suggest reading through Brian Vaughn's recent React blog post about this stuff: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

[cellog]

The current implementation is broken in 16.4 precisely because of the content of the article by Brian that you posted above. It's not possible to compare performance when one implementation doesn't work.

However, I do see your point on performance, because of the following:

sCU is invoked in 3 situations:

1. props change
2. local state changes
3. parent re-renders

I forgot about #3. However, this is easily fixed by adding a 1-line test at the top to see if props and state have changed. I don't understand why that makes this approach invalid?

sCU is the only instance method that is called when either props or state changes before render that is async-safe. gDSFP is intended only to construct local state from changes in props, which is not how the current implementation uses it, and that is why it breaks in 16.4

[cellog]

On further reflection, I think that you're right. The only thing we need sCU for is to detect props changes and local state change, so the answer is to use PureComponent for the wrapper and to move the derived props generation into render() and we can do a "did redux state change change the derived props?" test in the listener and cache those results. This way, we only re-generate derived props for prop changes or for redux state changes.

[cellog]

@timdorr I did another deep dive, and have found a few of things

1. shouldComponentUpdate is called 1 times less often as getDerivedStateFromProps (only on initialization is it not called and also if forceUpdate is called), so there is no performance difference from the old version in that regard. In other words, gDSFP is called any time the parent re-renders, when props change, or when local state updates. sCU does the same (https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-unconditionally-copying-props-to-state)
2. using PureComponent breaks several tests, because react-redux is supposed to also work with impure components beneath it. So that solution actually doesn't work.
3. what this means is that react-redux already was expected to re-run the mapStateToProps upon parent re-render, so the solution I've provided works.
4. I tried updating dev dependency for the master branch to 16.4, and the existing test suite had 12 failures that exactly match the behavior I saw in the wild, reported in [5.1.0-test.1] Connected components don't re render after state update #965
5. @markerikson had a concern in [5.1.0-test.1] Connected components don't re render after state update #965 (comment) that instance props were not async-safe, but that turns out to be untrue ([5.1.0-test.1] Connected components don't re render after state update #965 (comment))

So, I don't think you're right, I encourage you to take another look. Sorry for the churn, this is actually a rather vexing issue to solve right.

[cellog]

P.S. just confirmed in the wild that the PR fixes my app's issues

[markerikson]

Why is there a change in the number of times mapState is being called here?

[markerikson]

I've spent the last few hours trying to wrap my head around a bunch of stuff (lifecycle methods, some Suspense info, behavior of React-Redux, etc). To be honest, my head hurts and I think I'm feeling more confused than I was when I started.

I have a vague feeling that part of the issue here is that v5 currently tries to handle all the update work in one giant super-selector. But, that's largely due to the way that either changes in store state or own props can cascade through and change the output of mapState, mapDispatch, or even mergeProps.

Per this specific PR: doing work like that in sCU feels very wrong to me, but we were already doing all that work in cWRP originally, and cWRP always precedes sCU, so I think it may be the same perf-wise. (As a related note, we really ought to collect some of the various benchmark repos that have been floating around since v5 was put together, and put together some formal perf test suites somehow.)

I don't like keeping the current store state as an instance variable. If we're updating the component state anyway, we ought to include the store state along with the update count there.

I'm also concerned by the changes to the tests where we're bumping up the expected number of mapState calls. How do things behave currently in regards to passed props and mapState calls, and why are we calling mapState more now? That there does have some perf implications.

[cellog]

Those particular tests are odd. We have the entire state passing to props, and this is both in parent and child components.

Have you run the test suite for v5 in React 16.4? It may be a change in React rather than in react-redux (this just occurred to me, I haven't done it and also just woke up). If so, those tests will all fail in 16.4

In any case, what appears to be happening is that the first subscription runs, which immediately triggers a state change in the parent component, and that triggers sCU/render and then the prop change goes to the child component. Only then does the second subscription run.

I haven't taken a deep dive into how v5 works, I'll do that too, and then maybe we can get an answer as to why those tests changed. I did post my concern about those tests in the pull request comment for the same reason. It feels weird, but perhaps the perf implications only exist in those odd unrealistic tests? None of the other parent/child tests have additional mapState calls.

[markerikson]

I think it's because of the change from this:

      onStateChange() {
        this.selector.run(this.props)

        if (!this.selector.shouldComponentUpdate) {
          this.notifyNestedSubs()
        } else {
          this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate
          this.setState(dummyState)
        }
      }

to this:

      triggerUpdateOnStoreStateChange(callback = noop) {
        // runs when an action is dispatched by the store we are listening to
        // if the store state has changed, we save that and update the component state
        // to force a re-generation of derived props
        if (this.isUnmounted) {
          return
        }

        this.setState(prevState => {
          const newState = this.store.getState()
          if (this.storeState === newState) {
            return prevState
          }
          this.storeState = newState
          return {
            updateCount: prevState.updateCount++
          }
        }, callback)
      }

In the original v5 implementation, the subscription callback immediately checks to see if we need to re-render based on the new store state and the existing props. It then either notifies child subscribers right away if this component is fine, or notifies them after this component has updated. That way, the children never see the new store state but the old props from the parent.

The prior PR for this task changed the logic to only read the new store state inside a setState() updater function, and then notify children after the component state has been updated. As a result, the children will see both updated parent props and old store state, and then updated parent props and new store state.

That's bad, and we should figure out a different approach.

[cellog]

Nice, now that I understand how it did it before, I think I can make that happen. I'll take a look tomorrow morning when I have time. Thanks for the logic debug!

[cellog]

Superseded By #980