diff --git a/package-lock.json b/package-lock.json index 0cb5531f9..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,29 +6669,22 @@ "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-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 0a95d86b6..22e495b3b 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", @@ -86,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", 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..8721313b1 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++ @@ -2266,8 +2292,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 +2303,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', () => {