From aaa0470ea43578c2eda0e26f68fb6e35ab75e08d Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sat, 14 Jul 2018 16:47:53 -0400 Subject: [PATCH 1/3] fix connected props derived props generation This commit fixes https://github.com/reduxjs/react-redux/issues/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. --- package-lock.json | 5 -- package.json | 3 +- src/components/connectAdvanced.js | 113 +++++++++++++++++------------- test/components/Provider.spec.js | 31 ++++++-- test/components/connect.spec.js | 49 +++++++++++-- 5 files changed, 135 insertions(+), 66 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0cb5531f9..e91114a90 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6669,11 +6669,6 @@ "prop-types": "^15.6.0" } }, - "react-lifecycles-compat": { - "version": "3.0.4", - "resolved": "https://registry.npmjs.org/react-lifecycles-compat/-/react-lifecycles-compat-3.0.4.tgz", - "integrity": "sha512-fBASbA6LnOU9dOU2eW7aQ8xmYBSXUIWr+UmF9b1efZBazGNO+rcXT/icdKnYm2pTwcRylVUYwW7H1PHfLekVzA==" - }, "react-test-renderer": { "version": "16.3.2", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.3.2.tgz", diff --git a/package.json b/package.json index 0a95d86b6..8f1b37571 100644 --- a/package.json +++ b/package.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", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 1a5faa625..c4495b86a 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -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) } } diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index 2d96b6c85..078abb462 100644 --- a/test/components/Provider.spec.js +++ b/test/components/Provider.spec.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 without warnings', () => { diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 3fcf44bc4..d32231db5 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.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() + const a = TestRenderer.create() 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 }} + 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
{this.props.count}
}} @@ -2276,6 +2305,12 @@ describe('React', () => { TestRenderer.create() 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', () => { From 41ff984c04a15f1b79d8cf0389d862a4de12aac9 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sat, 14 Jul 2018 16:52:48 -0400 Subject: [PATCH 2/3] fix linting errors --- test/components/connect.spec.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index d32231db5..8721313b1 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2277,11 +2277,9 @@ describe('React', () => { } const store = createStore((state = 0, action) => (action.type === 'INC' ? state + 1 : state)) - const a = TestRenderer.create() + TestRenderer.create() expect(mapStateToProps).toHaveBeenCalledTimes(1) - console.log('dispatch') - console.log(a.getInstance().props.children) store.dispatch({ type: 'INC' }) expect(mapStateToProps).toHaveBeenCalledTimes(2) }) From 9f44bed4e80fb7b9e8585dc86987e897a417051c Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Mon, 16 Jul 2018 15:37:55 -0400 Subject: [PATCH 3/3] update dev dependency to React 16.4 --- package-lock.json | 34 ++++++++++++++++------------------ package.json | 6 +++--- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/package-lock.json b/package-lock.json index e91114a90..7d92b3966 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6646,9 +6646,9 @@ } }, "react": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react/-/react-16.3.2.tgz", - "integrity": "sha512-o5GPdkhciQ3cEph6qgvYB7LTOHw/GB0qRI6ZFNugj49qJCFfgHwVNjZ5u+b7nif4vOeMIOuYj3CeYe2IBD74lg==", + "version": "16.4.1", + "resolved": "https://registry.npmjs.org/react/-/react-16.4.1.tgz", + "integrity": "sha512-3GEs0giKp6E0Oh/Y9ZC60CmYgUPnp7voH9fbjWsvXtYFb4EWtgQub0ADSq0sJR0BbHc4FThLLtzlcFaFXIorwg==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -6658,9 +6658,9 @@ } }, "react-dom": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.3.2.tgz", - "integrity": "sha512-MMPko3zYncNrz/7gG17wJWUREZDvskZHXOwbttzl0F0L3wDmToyuETuo/r8Y5yvDejwYcRyWI1lvVBjLJWFwKA==", + "version": "16.4.1", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.4.1.tgz", + "integrity": "sha512-1Gin+wghF/7gl4Cqcvr1DxFX2Osz7ugxSwl6gBqCMpdrxHjIFUS7GYxrFftZ9Ln44FHw0JxCFD9YtZsrbR5/4A==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -6669,24 +6669,22 @@ "prop-types": "^15.6.0" } }, + "react-is": { + "version": "16.4.1", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.4.1.tgz", + "integrity": "sha512-xpb0PpALlFWNw/q13A+1aHeyJyLYCg0/cCHPUA43zYluZuIPHaHL3k8OBsTgQtxqW0FhyDEMvi8fZ/+7+r4OSQ==", + "dev": true + }, "react-test-renderer": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.3.2.tgz", - "integrity": "sha512-lL8WHIpCTMdSe+CRkt0rfMxBkJFyhVrpdQ54BaJRIrXf9aVmbeHbRA8GFRpTvohPN5tPzMabmrzW2PUfWCfWwQ==", + "version": "16.4.1", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.1.tgz", + "integrity": "sha512-wyyiPxRZOTpKnNIgUBOB6xPLTpIzwcQMIURhZvzUqZzezvHjaGNsDPBhMac5fIY3Jf5NuKxoGvV64zDSOECPPQ==", "dev": true, "requires": { "fbjs": "^0.8.16", "object-assign": "^4.1.1", "prop-types": "^15.6.0", - "react-is": "^16.3.2" - }, - "dependencies": { - "react-is": { - "version": "16.3.2", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.3.2.tgz", - "integrity": "sha512-ybEM7YOr4yBgFd6w8dJqwxegqZGJNBZl6U27HnGKuTZmDvVrD5quWOK/wAnMywiZzW+Qsk+l4X2c70+thp/A8Q==", - "dev": true - } + "react-is": "^16.4.1" } }, "read-pkg": { diff --git a/package.json b/package.json index 8f1b37571..22e495b3b 100644 --- a/package.json +++ b/package.json @@ -85,9 +85,9 @@ "eslint-plugin-react": "^7.9.1", "glob": "^7.1.1", "jest": "^23.1.0", - "react": "^16.3.2", - "react-dom": "^16.3.2", - "react-test-renderer": "^16.3.2", + "react": "^16.4.1", + "react-dom": "^16.4.1", + "react-test-renderer": "^16.4.1", "redux": "^4.0.0", "rimraf": "^2.6.2", "rollup": "^0.61.1",