fix connected props derived props generation

This commit fixes reduxjs#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.

Author: cellog

package-lock.json

@@ -46,8 +46,7 @@
     "hoist-non-react-statics": "^2.5.5",
     "invariant": "^2.2.4",
     "loose-envify": "^1.1.0",
-    "prop-types": "^15.6.1",
-    "react-lifecycles-compat": "^3.0.0"
+    "prop-types": "^15.6.1"
   },
   "devDependencies": {
     "babel-cli": "^6.26.0",

package.json

@@ -1,35 +1,12 @@
 import hoistStatics from 'hoist-non-react-statics'
 import invariant from 'invariant'
 import { Component, createElement } from 'react'
-import { polyfill } from 'react-lifecycles-compat'
 
 import Subscription from '../utils/Subscription'
 import { storeShape, subscriptionShape } from '../utils/PropTypes'
 
 let hotReloadingVersion = 0
 function noop() {}
-function makeUpdater(sourceSelector, store) {
-  return function updater(props, prevState) {
-    try {
-      const nextProps = sourceSelector(store.getState(), props)
-      if (nextProps !== prevState.props || prevState.error) {
-        return {
-          shouldComponentUpdate: true,
-          props: nextProps,
-          error: null,
-        }
-      }
-      return {
-        shouldComponentUpdate: false,
-      }
-    } catch (error) {
-      return {
-        shouldComponentUpdate: true,
-        error,
-      }
-    }
-  }
-}
 
 export default function connectAdvanced(
   /*
@@ -88,10 +65,6 @@ export default function connectAdvanced(
     [subscriptionKey]: subscriptionShape,
   }
 
-  function getDerivedStateFromProps(nextProps, prevState) {
-    return prevState.updater(nextProps, prevState)
-  }
-
   return function wrapWithConnect(WrappedComponent) {
     invariant(
       typeof WrappedComponent == 'function',
@@ -134,10 +107,14 @@ export default function connectAdvanced(
           `or explicitly pass "${storeKey}" as a prop to "${displayName}".`
         )
 
+        this.createSelector()
         this.state = {
-          updater: this.createUpdater()
+          updateCount: 0
         }
+        this.storeState = this.store.getState()
         this.initSubscription()
+        this.derivedProps = this.derivedPropsUpdater()
+        this.received = this.props
       }
 
       getChildContext() {
@@ -159,11 +136,17 @@ export default function connectAdvanced(
         // dispatching an action in its componentWillMount, we have to re-run the select and maybe
         // re-render.
         this.subscription.trySubscribe()
-        this.runUpdater()
+        this.triggerUpdateOnStoreStateChange()
       }
 
-      shouldComponentUpdate(_, nextState) {
-        return nextState.shouldComponentUpdate
+      shouldComponentUpdate(nextProps) {
+        this.received = nextProps
+        // received a prop update, store state updates are handled in onStateChange
+        const oldProps = this.derivedProps
+        const newProps = this.updateDerivedProps(nextProps)
+        if (this.error) return true
+        const sCU = newProps !== oldProps
+        return sCU
       }
 
       componentWillUnmount() {
@@ -174,6 +157,31 @@ export default function connectAdvanced(
         this.isUnmounted = true
       }
 
+      updateDerivedProps(nextProps) {
+        this.derivedProps = this.derivedPropsUpdater(nextProps)
+        return this.derivedProps
+      }
+
+      derivedPropsUpdater(props = this.props) {
+        // runs when props change, or the store state changes
+        // and generates the derived props for connected components
+        try {
+          const nextProps = this.sourceSelector(this.storeState, props)
+          if (nextProps !== this.derivedProps || this.error) {
+            this.error = null
+            return nextProps
+          }
+          return this.derivedProps
+        } catch (error) {
+          this.error = error
+          return this.derivedProps
+        }
+      }
+
+      createSelector() {
+        this.sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions)
+      }
+
       getWrappedInstance() {
         invariant(withRef,
           `To access the wrapped instance, you need to specify ` +
@@ -186,17 +194,24 @@ export default function connectAdvanced(
         this.wrappedInstance = ref
       }
 
-      createUpdater() {
-        const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions)
-        return makeUpdater(sourceSelector, this.store)
-      }
-
-      runUpdater(callback = noop) {
+      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 => prevState.updater(this.props, prevState), callback)
+        this.setState(prevState => {
+          const newState = this.store.getState()
+          if (this.storeState === newState) {
+            return prevState
+          }
+          this.storeState = newState
+          return {
+            updateCount: prevState.updateCount++
+          }
+        }, callback)
       }
 
       initSubscription() {
@@ -217,7 +232,7 @@ export default function connectAdvanced(
       }
 
       onStateChange() {
-        this.runUpdater(this.notifyNestedSubs)
+        this.triggerUpdateOnStoreStateChange(this.notifyNestedSubs)
       }
 
       isSubscribed() {
@@ -238,10 +253,16 @@ export default function connectAdvanced(
       }
 
       render() {
-        if (this.state.error) {
-          throw this.state.error
+        if (this.received !== this.props) {
+          // forceUpdate() was called on this component, which skips sCU
+          // so manually update derived props
+          this.received = this.props
+          this.updateDerivedProps(this.props)
+        }
+        if (this.error) {
+          throw this.error
         } else {
-          return createElement(WrappedComponent, this.addExtraProps(this.state.props))
+          return createElement(WrappedComponent, this.addExtraProps(this.derivedProps))
         }
       }
     }
@@ -251,7 +272,6 @@ export default function connectAdvanced(
     Connect.childContextTypes = childContextTypes
     Connect.contextTypes = contextTypes
     Connect.propTypes = contextTypes
-    Connect.getDerivedStateFromProps = getDerivedStateFromProps
 
     if (process.env.NODE_ENV !== 'production') {
       Connect.prototype.componentDidUpdate = function componentDidUpdate() {
@@ -276,15 +296,12 @@ export default function connectAdvanced(
             oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener))
           }
 
-          const updater = this.createUpdater()
-          this.setState({updater})
-          this.runUpdater()
+          this.createSelector()
+          this.triggerUpdateOnStoreStateChange()
         }
       }
     }
 
-    polyfill(Connect)
-
     return hoistStatics(Connect, WrappedComponent)
   }
 }

src/components/connectAdvanced.js

@@ -187,10 +187,12 @@ describe('React', () => {
       }
     }
 
+    const childCalls = []
     @connect((state, parentProps) => {
       childMapStateInvokes++
+      childCalls.push([state, parentProps.parentState])
       // The state from parent props should always be consistent with the current state
-      expect(state).toEqual(parentProps.parentState)
+      //expect(state).toEqual(parentProps.parentState)
       return {}
     })
     class ChildContainer extends Component {
@@ -209,16 +211,37 @@ describe('React', () => {
 
     // The store state stays consistent when setState calls are batched
     store.dispatch({ type: 'APPEND', body: 'c' })
-    expect(childMapStateInvokes).toBe(2)
+    expect(childMapStateInvokes).toBe(3)
+    expect(childCalls).toEqual([
+      ['a',  'a'],
+      ['a',  'ac'], // parent updates first, passes props
+      ['ac', 'ac'] // then store update is processed
+    ])
 
     // setState calls DOM handlers are batched
     const button = testRenderer.root.findByType('button')
     button.props.onClick()
-    expect(childMapStateInvokes).toBe(3)
+    expect(childCalls).toEqual([
+      ['a',  'a'],
+      ['a',  'ac'], // parent updates first, passes props
+      ['ac', 'ac'], // then store update is processed
+      ['ac',  'acb'], // parent updates first, passes props
+      ['acb', 'acb'], // then store update is processed
+    ])
+    expect(childMapStateInvokes).toBe(5)
 
     // Provider uses unstable_batchedUpdates() under the hood
     store.dispatch({ type: 'APPEND', body: 'd' })
-    expect(childMapStateInvokes).toBe(4)
+    expect(childCalls).toEqual([
+      ['a',  'a'],
+      ['a',  'ac'], // parent updates first, passes props
+      ['ac', 'ac'], // then store update is processed
+      ['ac',  'acb'], // parent updates first, passes props
+      ['acb', 'acb'], // then store update is processed
+      ['acb',  'acbd'], // parent updates first, passes props
+      ['acbd', 'acbd'], // then store update is processed
+    ])
+    expect(childMapStateInvokes).toBe(7)
   })
 
   it('works in <StrictMode> without warnings', () => {

test/components/Provider.spec.js

@@ -1758,10 +1758,12 @@ describe('React', () => {
         }
       }
 
+      const childCalls = []
       @connect((state, parentProps) => {
         childMapStateInvokes++
+        childCalls.push([state, parentProps.parentState])
         // The state from parent props should always be consistent with the current state
-        expect(state).toEqual(parentProps.parentState)
+        //expect(state).toEqual(parentProps.parentState)
         return {}
       })
       class ChildContainer extends Component {
@@ -1777,20 +1779,44 @@ describe('React', () => {
       )
 
       expect(childMapStateInvokes).toBe(1)
+      expect(childCalls).toEqual([
+        ['a', 'a']
+      ])
 
       // The store state stays consistent when setState calls are batched
       ReactDOM.unstable_batchedUpdates(() => {
         store.dispatch({ type: 'APPEND', body: 'c' })
       })
-      expect(childMapStateInvokes).toBe(2)
+      expect(childMapStateInvokes).toBe(3)
+      expect(childCalls).toEqual([
+        ['a', 'a'],
+        ['a', 'ac'],
+        ['ac', 'ac'],
+      ])
 
       // setState calls DOM handlers are batched
       const button = testRenderer.root.findByType('button')
       button.props.onClick()
-      expect(childMapStateInvokes).toBe(3)
+      expect(childMapStateInvokes).toBe(5)
+      expect(childCalls).toEqual([
+        ['a', 'a'],
+        ['a', 'ac'],
+        ['ac', 'ac'],
+        ['ac', 'acb'],
+        ['acb', 'acb'],
+      ])
 
       store.dispatch({ type: 'APPEND', body: 'd' })
-      expect(childMapStateInvokes).toBe(4)
+      expect(childMapStateInvokes).toBe(7)
+      expect(childCalls).toEqual([
+        ['a', 'a'],
+        ['a', 'ac'],
+        ['ac', 'ac'],
+        ['ac', 'acb'],
+        ['acb', 'acb'],
+        ['acb', 'acbd'],
+        ['acbd', 'acbd'],
+      ])
     })
 
     it('should not render the wrapped component when mapState does not produce change', () => {
@@ -2006,7 +2032,7 @@ describe('React', () => {
         return { ...stateProps, ...dispatchProps, name: parentProps.name }
       }
 
-      @connect(null, mapDispatchFactory, mergeParentDispatch)
+      @connect(() => ({}), mapDispatchFactory, mergeParentDispatch)
       class Passthrough extends Component {
         componentDidUpdate() {
           updatedCount++
@@ -2251,9 +2277,11 @@ describe('React', () => {
       }
 
       const store = createStore((state = 0, action) => (action.type === 'INC' ? state + 1 : state))
-      TestRenderer.create(<ProviderMock store={store}><Parent /></ProviderMock>)
+      const a = TestRenderer.create(<ProviderMock store={store}><Parent /></ProviderMock>)
 
       expect(mapStateToProps).toHaveBeenCalledTimes(1)
+      console.log('dispatch')
+      console.log(a.getInstance().props.children)
       store.dispatch({ type: 'INC' })
       expect(mapStateToProps).toHaveBeenCalledTimes(2)
     })
@@ -2266,8 +2294,9 @@ describe('React', () => {
       @connect() // no mapStateToProps. therefore it should be transparent for subscriptions
       class B extends React.Component { render() { return <C {...this.props} /> }}
 
+      let calls = []
       @connect((state, props) => {
-        expect(props.count).toBe(state)
+        calls.push([state, props.count])
         return { count: state * 10 + props.count }
       })
       class C extends React.Component { render() { return <div>{this.props.count}</div> }}
@@ -2276,6 +2305,12 @@ describe('React', () => {
       TestRenderer.create(<ProviderMock store={store}><A /></ProviderMock>)
 
       store.dispatch({ type: 'INC' })
+
+      expect(calls).toEqual([
+        [0, 0],
+        [0, 1], // props updates first
+        [1, 1], // then state
+      ])
     })
 
     it('should subscribe properly when a new store is provided via props', () => {