From b631441b6dfa226eead9597b267064207265068f Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Fri, 20 Jul 2018 20:24:03 -0400 Subject: [PATCH 01/19] up min react versions --- 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 0cb5531f9..e3e97df09 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,27 @@ "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-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", - "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..711fbbc17 100644 --- a/package.json +++ b/package.json @@ -86,9 +86,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", From 25e3fc6d956ca9fff6b9eba943031d5129fc7f82 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sat, 21 Jul 2018 16:13:24 -0400 Subject: [PATCH 02/19] working, except the issue with multiple mapState calls 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 --- package.json | 3 +- src/components/connectAdvanced.js | 117 ++++++++++++++++++------------ test/components/connect.spec.js | 8 +- 3 files changed, 78 insertions(+), 50 deletions(-) diff --git a/package.json b/package.json index 711fbbc17..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", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 1a5faa625..88c4ef898 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,35 +1,13 @@ import hoistStatics from 'hoist-non-react-statics' import invariant from 'invariant' import { Component, createElement } from 'react' -import { polyfill } from 'react-lifecycles-compat' +import shallowEqual from '../utils/shallowEqual' 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 +66,6 @@ export default function connectAdvanced( [subscriptionKey]: subscriptionShape, } - function getDerivedStateFromProps(nextProps, prevState) { - return prevState.updater(nextProps, prevState) - } - return function wrapWithConnect(WrappedComponent) { invariant( typeof WrappedComponent == 'function', @@ -124,22 +98,55 @@ export default function connectAdvanced( this.version = version this.renderCount = 0 - this.store = props[storeKey] || context[storeKey] + const store = props[storeKey] || context[storeKey] this.propsMode = Boolean(props[storeKey]) this.setWrappedInstance = this.setWrappedInstance.bind(this) - invariant(this.store, + invariant(store, `Could not find "${storeKey}" in either the context or props of ` + `"${displayName}". Either wrap the root component in a , ` + `or explicitly pass "${storeKey}" as a prop to "${displayName}".` ) + const storeState = store.getState() + const childPropsSelector = this.createChildSelector(store) + this.state = { + props, + childPropsSelector, + childProps: {}, + store, + storeState, + error: null, + } this.state = { - updater: this.createUpdater() + ...this.state, + ...Connect.getChildPropsState(props, this.state, ) + //(a, b) => console.log('constructor', WrappedComponent.name, a, b)) } this.initSubscription() } + static getChildPropsState(props, state, debug = false) { + try { + const nextProps = state.childPropsSelector(state.storeState, props) + if (nextProps === state.childProps) return null + if (debug) debug(nextProps, state.childProps) + return { childProps: nextProps } + } catch (error) { + if (debug) debug(error, state.childProps) + return { error } + } + } + + static getDerivedStateFromProps(props, state) { + if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null + return { + ...Connect.getChildPropsState(props, state, ) + //(a, b) => console.log('gDSFP', WrappedComponent.name, a, b)), + props: props + } + } + getChildContext() { // If this component received store from props, its subscription should be transparent // to any descendants receiving store+subscription from context; it passes along @@ -159,11 +166,14 @@ 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.updateChildPropsFromReduxStore(false) } shouldComponentUpdate(_, nextState) { - return nextState.shouldComponentUpdate + if (!connectOptions['pure']) { + return true + } + return nextState.childProps !== this.state.childProps || nextState.error } componentWillUnmount() { @@ -186,17 +196,35 @@ export default function connectAdvanced( this.wrappedInstance = ref } - createUpdater() { - const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions) - return makeUpdater(sourceSelector, this.store) + createChildSelector(store = this.state.store) { + return selectorFactory(store.dispatch, selectorFactoryOptions) } - runUpdater(callback = noop) { + updateChildPropsFromReduxStore(notify = true) { if (this.isUnmounted) { return } - this.setState(prevState => prevState.updater(this.props, prevState), callback) + this.setState(prevState => { + const nextState = this.state.store.getState() + if (nextState === prevState.storeState) { + if (notify) this.notifyNestedSubs() + return null + } + const childPropsState = Connect.getChildPropsState(this.state.props, { + ...this.state, + storeState: nextState + },) + //(a, b) => console.log('redux', WrappedComponent.name, a, b)) + const ret = { + storeState: nextState, + ...childPropsState + } + if (notify && childPropsState === null) { + this.notifyNestedSubs() + } + return ret + }, notify ? this.notifyNestedSubs : undefined) } initSubscription() { @@ -205,7 +233,7 @@ export default function connectAdvanced( // 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. const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] - this.subscription = new Subscription(this.store, parentSub, this.onStateChange.bind(this)) + this.subscription = new Subscription(this.state.store, parentSub, this.onStateChange.bind(this)) // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `this.subscription` will then be null. An @@ -217,7 +245,7 @@ export default function connectAdvanced( } onStateChange() { - this.runUpdater(this.notifyNestedSubs) + this.updateChildPropsFromReduxStore() } isSubscribed() { @@ -241,7 +269,7 @@ export default function connectAdvanced( if (this.state.error) { throw this.state.error } else { - return createElement(WrappedComponent, this.addExtraProps(this.state.props)) + return createElement(WrappedComponent, this.addExtraProps(this.state.childProps)) } } } @@ -251,7 +279,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 +303,13 @@ export default function connectAdvanced( oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) } - const updater = this.createUpdater() - this.setState({updater}) - this.runUpdater() + const childPropsSelector = this.createChildSelector() + const childProps = childPropsSelector(this.state.props, this.state.storeState) + this.setState({ childPropsSelector, childProps }) } } } - polyfill(Connect) - return hoistStatics(Connect, WrappedComponent) } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 3fcf44bc4..1995ccdba 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2266,8 +2266,9 @@ describe('React', () => { @connect() // no mapStateToProps. therefore it should be transparent for subscriptions class B extends React.Component { render() { return }} + const 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 +2277,9 @@ describe('React', () => { TestRenderer.create() store.dispatch({ type: 'INC' }) + expect(calls).toEqual([ + + ]) }) it('should subscribe properly when a new store is provided via props', () => { @@ -2362,7 +2366,7 @@ describe('React', () => { ) const container = testRenderer.root.findByType(Container) - expect(container.instance.store).toBe(store) + expect(container.instance.state.store).toBe(store) }) }) }) From de4e4f319b600b29fcfea7354cec5ef4e2f665f2 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sat, 21 Jul 2018 16:17:26 -0400 Subject: [PATCH 03/19] typo --- src/components/connectAdvanced.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 88c4ef898..bb8de4698 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -141,7 +141,7 @@ export default function connectAdvanced( static getDerivedStateFromProps(props, state) { if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null return { - ...Connect.getChildPropsState(props, state, ) + ...Connect.getChildPropsState(props, state, ), //(a, b) => console.log('gDSFP', WrappedComponent.name, a, b)), props: props } From f751ac95487ccb4a19756d9cddd0fca1cd5f466b Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sun, 22 Jul 2018 16:20:00 -0400 Subject: [PATCH 04/19] success! gDSFP-based react-redux 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. --- package-lock.json | 5 ----- src/components/connectAdvanced.js | 26 ++++++++++---------------- test/components/connect.spec.js | 6 +----- 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/package-lock.json b/package-lock.json index e3e97df09..7d92b3966 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6675,11 +6675,6 @@ "integrity": "sha512-xpb0PpALlFWNw/q13A+1aHeyJyLYCg0/cCHPUA43zYluZuIPHaHL3k8OBsTgQtxqW0FhyDEMvi8fZ/+7+r4OSQ==", "dev": true }, - "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.4.1", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.1.tgz", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index bb8de4698..cd3490ab4 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -120,31 +120,25 @@ export default function connectAdvanced( } this.state = { ...this.state, - ...Connect.getChildPropsState(props, this.state, ) - //(a, b) => console.log('constructor', WrappedComponent.name, a, b)) + ...Connect.getChildPropsState(props, this.state) } this.initSubscription() } static getChildPropsState(props, state, debug = false) { try { - const nextProps = state.childPropsSelector(state.storeState, props) + const nextProps = state.childPropsSelector(state.store.getState(), props) if (nextProps === state.childProps) return null - if (debug) debug(nextProps, state.childProps) return { childProps: nextProps } } catch (error) { - if (debug) debug(error, state.childProps) return { error } } } static getDerivedStateFromProps(props, state) { if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null - return { - ...Connect.getChildPropsState(props, state, ), - //(a, b) => console.log('gDSFP', WrappedComponent.name, a, b)), - props: props - } + const nextChildProps = Connect.getChildPropsState(props, state) + return nextChildProps } getChildContext() { @@ -208,14 +202,12 @@ export default function connectAdvanced( this.setState(prevState => { const nextState = this.state.store.getState() if (nextState === prevState.storeState) { - if (notify) this.notifyNestedSubs() return null } - const childPropsState = Connect.getChildPropsState(this.state.props, { + const childPropsState = Connect.getChildPropsState(this.props, { ...this.state, storeState: nextState - },) - //(a, b) => console.log('redux', WrappedComponent.name, a, b)) + }) const ret = { storeState: nextState, ...childPropsState @@ -224,7 +216,9 @@ export default function connectAdvanced( this.notifyNestedSubs() } return ret - }, notify ? this.notifyNestedSubs : undefined) + }, () => { + if (notify) this.notifyNestedSubs() + }) } initSubscription() { @@ -304,7 +298,7 @@ export default function connectAdvanced( } const childPropsSelector = this.createChildSelector() - const childProps = childPropsSelector(this.state.props, this.state.storeState) + const childProps = childPropsSelector(this.props, this.state.storeState) this.setState({ childPropsSelector, childProps }) } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 1995ccdba..f8e49f8f9 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2266,9 +2266,8 @@ describe('React', () => { @connect() // no mapStateToProps. therefore it should be transparent for subscriptions class B extends React.Component { render() { return }} - const calls = [] @connect((state, props) => { - calls.push([state, props.count]) + expect(props.count).toBe(state) return { count: state * 10 + props.count } }) class C extends React.Component { render() { return
{this.props.count}
}} @@ -2277,9 +2276,6 @@ describe('React', () => { TestRenderer.create() store.dispatch({ type: 'INC' }) - expect(calls).toEqual([ - - ]) }) it('should subscribe properly when a new store is provided via props', () => { From 928489473009472b4e81754c61e4337e2414bb8f Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sun, 22 Jul 2018 16:21:37 -0400 Subject: [PATCH 05/19] up the peer dependency to React 16.4 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 22e495b3b..7ca7a7ca1 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "coverage": "codecov" }, "peerDependencies": { - "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0", + "react": "^16.4.0-0", "redux": "^2.0.0 || ^3.0.0 || ^4.0.0-0" }, "dependencies": { From 948c20fb85f470cfd36563d62712514a3be9e38e Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Sun, 22 Jul 2018 16:26:16 -0400 Subject: [PATCH 06/19] remove debugging code --- src/components/connectAdvanced.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index cd3490ab4..c6a6f9b53 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -125,7 +125,7 @@ export default function connectAdvanced( this.initSubscription() } - static getChildPropsState(props, state, debug = false) { + static getChildPropsState(props, state) { try { const nextProps = state.childPropsSelector(state.store.getState(), props) if (nextProps === state.childProps) return null From 490b0e35d95a96563922c48b76d8f0dd514c9290 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Tue, 24 Jul 2018 17:50:54 -0400 Subject: [PATCH 07/19] use polyfill, make peer dependency relaxed Store last seen props in gDSFP (was an oversight). Does not change behavior, just performance. --- package-lock.json | 5 +++++ package.json | 5 +++-- src/components/connectAdvanced.js | 7 ++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7d92b3966..e3e97df09 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6675,6 +6675,11 @@ "integrity": "sha512-xpb0PpALlFWNw/q13A+1aHeyJyLYCg0/cCHPUA43zYluZuIPHaHL3k8OBsTgQtxqW0FhyDEMvi8fZ/+7+r4OSQ==", "dev": true }, + "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.4.1", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.1.tgz", diff --git a/package.json b/package.json index 7ca7a7ca1..56636aad3 100644 --- a/package.json +++ b/package.json @@ -39,14 +39,15 @@ "coverage": "codecov" }, "peerDependencies": { - "react": "^16.4.0-0", + "react": "^0.14.0 || ^15.0.0-0 || ^16.0.0-0", "redux": "^2.0.0 || ^3.0.0 || ^4.0.0-0" }, "dependencies": { "hoist-non-react-statics": "^2.5.5", "invariant": "^2.2.4", "loose-envify": "^1.1.0", - "prop-types": "^15.6.1" + "prop-types": "^15.6.1", + "react-lifecycles-compat": "^3.0.4" }, "devDependencies": { "babel-cli": "^6.26.0", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index c6a6f9b53..6ddf8e1d3 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,6 +1,7 @@ import hoistStatics from 'hoist-non-react-statics' import invariant from 'invariant' import { Component, createElement } from 'react' +import { polyfill } from 'react-lifecycles-compat' import shallowEqual from '../utils/shallowEqual' import Subscription from '../utils/Subscription' @@ -138,7 +139,10 @@ export default function connectAdvanced( static getDerivedStateFromProps(props, state) { if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null const nextChildProps = Connect.getChildPropsState(props, state) - return nextChildProps + return { + ...nextChildProps, + props + } } getChildContext() { @@ -273,6 +277,7 @@ export default function connectAdvanced( Connect.childContextTypes = childContextTypes Connect.contextTypes = contextTypes Connect.propTypes = contextTypes + polyfill(Connect) if (process.env.NODE_ENV !== 'production') { Connect.prototype.componentDidUpdate = function componentDidUpdate() { From 86e986b0fea1febea668da65c041303967636442 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Wed, 25 Jul 2018 06:43:51 -0400 Subject: [PATCH 08/19] fix style details --- src/components/connectAdvanced.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 6ddf8e1d3..60a7ffa82 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -137,7 +137,7 @@ export default function connectAdvanced( } static getDerivedStateFromProps(props, state) { - if (connectOptions.pure && shallowEqual(props, state.props) || state.error) return null + if ((connectOptions.pure && shallowEqual(props, state.props)) || state.error) return null const nextChildProps = Connect.getChildPropsState(props, state) return { ...nextChildProps, @@ -168,7 +168,7 @@ export default function connectAdvanced( } shouldComponentUpdate(_, nextState) { - if (!connectOptions['pure']) { + if (!connectOptions.pure) { return true } return nextState.childProps !== this.state.childProps || nextState.error @@ -231,7 +231,7 @@ export default function connectAdvanced( // 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. const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] - this.subscription = new Subscription(this.state.store, parentSub, this.onStateChange.bind(this)) + this.subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `this.subscription` will then be null. An @@ -242,10 +242,6 @@ export default function connectAdvanced( this.notifyNestedSubs = this.subscription.notifyNestedSubs.bind(this.subscription) } - onStateChange() { - this.updateChildPropsFromReduxStore() - } - isSubscribed() { return Boolean(this.subscription) && this.subscription.isSubscribed() } @@ -288,7 +284,7 @@ export default function connectAdvanced( // If any connected descendants don't hot reload (and resubscribe in the process), their // listeners will be lost when we unsubscribe. Unfortunately, by copying over all // listeners, this does mean that the old versions of connected descendants will still be - // notified of state changes; however, their onStateChange function is a no-op so this + // notified of state changes; however, their updateChildPropsFromReduxStore function is a no-op so this // isn't a huge deal. let oldListeners = []; From 1d96dac430300a888aaa8b755ac9ca5dbfd369e1 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Wed, 1 Aug 2018 00:24:11 -0400 Subject: [PATCH 09/19] move instance props into state This does not make subscriptions async-safe, because redux uses the subscription to render children. This is an anti-pattern that will need to be fixed in 6.0 when a redesign using the new Context API is done --- src/components/connectAdvanced.js | 43 +++++++++++++++++-------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 60a7ffa82..dd1c1282e 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -150,7 +150,7 @@ export default function connectAdvanced( // to any descendants receiving store+subscription from context; it passes along // subscription passed to it. Otherwise, it shadows the parent subscription, which allows // Connect to control ordering of notifications to flow top-down. - const subscription = this.propsMode ? null : this.subscription + const subscription = this.propsMode ? null : this.state.subscription return { [subscriptionKey]: subscription || this.context[subscriptionKey] } } @@ -163,7 +163,7 @@ export default function connectAdvanced( // 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.subscription.trySubscribe() + this.state.subscription.trySubscribe() this.updateChildPropsFromReduxStore(false) } @@ -175,11 +175,13 @@ export default function connectAdvanced( } componentWillUnmount() { - if (this.subscription) this.subscription.tryUnsubscribe() - this.subscription = null - this.notifyNestedSubs = noop - this.store = null + if (this.state.subscription) this.state.subscription.tryUnsubscribe() this.isUnmounted = true + this.setState({ + subscription: null, + store: null, + notifyNestedSubs: noop + }) } getWrappedInstance() { @@ -217,37 +219,40 @@ export default function connectAdvanced( ...childPropsState } if (notify && childPropsState === null) { - this.notifyNestedSubs() + this.state.notifyNestedSubs() } return ret }, () => { - if (notify) this.notifyNestedSubs() + if (notify) this.state.notifyNestedSubs() }) } + // Because this function is called from the constructor, we have to mutate state direc initSubscription() { if (!shouldHandleStateChanges) return // 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. const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] - this.subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) + // eslint-disable-next-line + this.state.subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in - // the middle of the notification loop, where `this.subscription` will then be null. An + // the middle of the notification loop, where `this.state.subscription` will then be null. An // extra null check every change can be avoided by copying the method onto `this` and then // replacing it with a no-op on unmount. This can probably be avoided if Subscription's // listeners logic is changed to not call listeners that have been unsubscribed in the // middle of the notification loop. - this.notifyNestedSubs = this.subscription.notifyNestedSubs.bind(this.subscription) + // eslint-disable-next-line + this.state.notifyNestedSubs = this.state.subscription.notifyNestedSubs.bind(this.state.subscription) } isSubscribed() { - return Boolean(this.subscription) && this.subscription.isSubscribed() + return Boolean(this.state.subscription) && this.state.subscription.isSubscribed() } addExtraProps(props) { - if (!withRef && !renderCountProp && !(this.propsMode && this.subscription)) return props + if (!withRef && !renderCountProp && !(this.propsMode && this.state.subscription)) return props // make a shallow copy so that fields added don't leak to the original selector. // this is especially important for 'ref' since that's a reference back to the component // instance. a singleton memoized selector would then be holding a reference to the @@ -255,7 +260,7 @@ export default function connectAdvanced( const withExtras = { ...props } if (withRef) withExtras.ref = this.setWrappedInstance if (renderCountProp) withExtras[renderCountProp] = this.renderCount++ - if (this.propsMode && this.subscription) withExtras[subscriptionKey] = this.subscription + if (this.propsMode && this.state.subscription) withExtras[subscriptionKey] = this.state.subscription return withExtras } @@ -288,14 +293,14 @@ export default function connectAdvanced( // isn't a huge deal. let oldListeners = []; - if (this.subscription) { - oldListeners = this.subscription.listeners.get() - this.subscription.tryUnsubscribe() + if (this.state.subscription) { + oldListeners = this.state.subscription.listeners.get() + this.state.subscription.tryUnsubscribe() } this.initSubscription() if (shouldHandleStateChanges) { - this.subscription.trySubscribe() - oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) + this.state.subscription.trySubscribe() + oldListeners.forEach(listener => this.state.subscription.listeners.subscribe(listener)) } const childPropsSelector = this.createChildSelector() From 26e4fb840d03e3cb7f3593e536db3c6d4e561bd0 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Wed, 1 Aug 2018 21:39:22 -0400 Subject: [PATCH 10/19] make subscription async-safe In redux 5 and earlier, subscription to the store is done in the constructor, and passed down to children. But in async, it is possible for a render tree to begin, then suspend, then be discarded. In this case, we leak memory from the subscription that was created. This new system postpones the creation of subscription until componentDidMount is called, which is when subscriptions are safe to be made. The principles are gleaned from this gist by bvaughn: https://gist.github.com/bvaughn/d569177d70b50b58bff69c3c4a5353f3 --- src/components/connectAdvanced.js | 88 +++++++++++++++++++------------ test/components/connect.spec.js | 2 +- 2 files changed, 54 insertions(+), 36 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index dd1c1282e..7e7183e16 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -118,12 +118,14 @@ export default function connectAdvanced( store, storeState, error: null, + subscription: null, + lastSub: null, + notifyNestedSubs: noop } this.state = { ...this.state, ...Connect.getChildPropsState(props, this.state) } - this.initSubscription() } static getChildPropsState(props, state) { @@ -137,11 +139,19 @@ export default function connectAdvanced( } static getDerivedStateFromProps(props, state) { - if ((connectOptions.pure && shallowEqual(props, state.props)) || state.error) return null + let ret = null + if (state.lastSub !== state.subscription) { + ret = { lastSub: state.subscription } + if (shallowEqual(props, state.props) || state.error) { + return ret + } + } + if ((connectOptions.pure && shallowEqual(props, state.props)) || state.error) return ret const nextChildProps = Connect.getChildPropsState(props, state) return { ...nextChildProps, - props + props, + lastSub: state.subscription } } @@ -155,16 +165,7 @@ export default function connectAdvanced( } componentDidMount() { - if (!shouldHandleStateChanges) return - - // componentWillMount fires during server side rendering, but componentDidMount and - // componentWillUnmount do not. Because of this, trySubscribe happens during ...didMount. - // Otherwise, unsubscription would never take place during SSR, causing a memory leak. - // 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.state.subscription.trySubscribe() - this.updateChildPropsFromReduxStore(false) + return this.updateSubscription() } shouldComponentUpdate(_, nextState) { @@ -184,6 +185,39 @@ export default function connectAdvanced( }) } + updateSubscription() { + if (!shouldHandleStateChanges) return + + this.setState(state => { + if (state.subscription) 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. + const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] + const subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) + subscription.trySubscribe() + return { + subscription, + lastSub: state.subscription, + + // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in + // the middle of the notification loop, where `this.state.subscription` will then be null. An + // extra null check every change can be avoided by copying the method onto `this` and then + // replacing it with a no-op on unmount. This can probably be avoided if Subscription's + // listeners logic is changed to not call listeners that have been unsubscribed in the + // middle of the notification loop. + notifyNestedSubs: subscription.notifyNestedSubs.bind(subscription) + } + }, () => { + // componentWillMount fires during server side rendering, but componentDidMount and + // componentWillUnmount do not. Because of this, trySubscribe happens during ...didMount. + // Otherwise, unsubscription would never take place during SSR, causing a memory leak. + // 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) + }) + } + getWrappedInstance() { invariant(withRef, `To access the wrapped instance, you need to specify ` + @@ -227,26 +261,6 @@ export default function connectAdvanced( }) } - // Because this function is called from the constructor, we have to mutate state direc - initSubscription() { - if (!shouldHandleStateChanges) return - - // 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. - const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] - // eslint-disable-next-line - this.state.subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) - - // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in - // the middle of the notification loop, where `this.state.subscription` will then be null. An - // extra null check every change can be avoided by copying the method onto `this` and then - // replacing it with a no-op on unmount. This can probably be avoided if Subscription's - // listeners logic is changed to not call listeners that have been unsubscribed in the - // middle of the notification loop. - // eslint-disable-next-line - this.state.notifyNestedSubs = this.state.subscription.notifyNestedSubs.bind(this.state.subscription) - } - isSubscribed() { return Boolean(this.state.subscription) && this.state.subscription.isSubscribed() } @@ -297,17 +311,21 @@ export default function connectAdvanced( oldListeners = this.state.subscription.listeners.get() this.state.subscription.tryUnsubscribe() } - this.initSubscription() if (shouldHandleStateChanges) { - this.state.subscription.trySubscribe() + //this.state.subscription.trySubscribe() + this.updateSubscription() oldListeners.forEach(listener => this.state.subscription.listeners.subscribe(listener)) } const childPropsSelector = this.createChildSelector() const childProps = childPropsSelector(this.props, this.state.storeState) this.setState({ childPropsSelector, childProps }) + } else { + this.updateSubscription() } } + } else { + Connect.prototype.componentDidUpdate = Connect.prototype.updateSubscription } return hoistStatics(Connect, WrappedComponent) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index f8e49f8f9..c80fffa11 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1866,7 +1866,7 @@ describe('React', () => { store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(4) expect(renderCalls).toBe(2) - expect(spy).toHaveBeenCalledTimes(3) + expect(spy).toHaveBeenCalledTimes(5) // 2 times are subscription setting spy.mockRestore() }) From 6b83e131c2105707cef24c98004fc2e5dfec6d09 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Thu, 2 Aug 2018 14:12:07 -0400 Subject: [PATCH 11/19] fully fix subscription async-safe Subscription now works as follows: * on element creation, a Subscription object is initialized, but does not link to anything outside of the current element * after the component tree is rendered, componentDidMount is called * This triggers hydration of the subscription tree, which is ready to subscribe * parent elements create their listener collection, and children subscribe cascading to the bottom of the element tree --- .eslintrc | 3 ++- src/components/connectAdvanced.js | 30 ++++++++++++--------- src/utils/Subscription.js | 45 +++++++++++++++++++++++++++---- test/components/connect.spec.js | 2 ++ 4 files changed, 61 insertions(+), 19 deletions(-) diff --git a/.eslintrc b/.eslintrc index 05527f778..7b11d5141 100644 --- a/.eslintrc +++ b/.eslintrc @@ -16,7 +16,8 @@ "env": { "browser": true, "mocha": true, - "node": true + "node": true, + "es6": true }, "rules": { "valid-jsdoc": 2, diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 7e7183e16..dead254c4 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -109,7 +109,7 @@ export default function connectAdvanced( `or explicitly pass "${storeKey}" as a prop to "${displayName}".` ) const storeState = store.getState() - + const subscription = (this.propsMode ? null : context[subscriptionKey]) const childPropsSelector = this.createChildSelector(store) this.state = { props, @@ -118,7 +118,7 @@ export default function connectAdvanced( store, storeState, error: null, - subscription: null, + subscription: new Subscription(store, subscription, this.updateChildPropsFromReduxStore.bind(this)), lastSub: null, notifyNestedSubs: noop } @@ -185,18 +185,15 @@ export default function connectAdvanced( }) } - updateSubscription() { + updateSubscription(hotReloadCallback) { if (!shouldHandleStateChanges) return this.setState(state => { - if (state.subscription) return null + 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. - const parentSub = (this.propsMode ? this.props : this.context)[subscriptionKey] - const subscription = new Subscription(this.state.store, parentSub, this.updateChildPropsFromReduxStore.bind(this)) - subscription.trySubscribe() + this.state.subscription.hydrate() return { - subscription, lastSub: state.subscription, // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in @@ -205,9 +202,12 @@ export default function connectAdvanced( // replacing it with a no-op on unmount. This can probably be avoided if Subscription's // listeners logic is changed to not call listeners that have been unsubscribed in the // middle of the notification loop. - notifyNestedSubs: subscription.notifyNestedSubs.bind(subscription) + notifyNestedSubs: this.state.subscription.notifyNestedSubs.bind(this.state.subscription) } }, () => { + if (hotReloadCallback) { + hotReloadCallback() + } // componentWillMount fires during server side rendering, but componentDidMount and // componentWillUnmount do not. Because of this, trySubscribe happens during ...didMount. // Otherwise, unsubscription would never take place during SSR, causing a memory leak. @@ -308,13 +308,17 @@ export default function connectAdvanced( let oldListeners = []; if (this.state.subscription) { - oldListeners = this.state.subscription.listeners.get() + if (this.state.subscription.listeners.get) { + // only retrieve listeners if subscription has completed. If hot reload occurs immediately + // the subscription has not yet happened, so we don't need to retrieve old listeners + oldListeners = this.state.subscription.listeners.get() + } this.state.subscription.tryUnsubscribe() } if (shouldHandleStateChanges) { - //this.state.subscription.trySubscribe() - this.updateSubscription() - oldListeners.forEach(listener => this.state.subscription.listeners.subscribe(listener)) + this.updateSubscription(() => { + oldListeners.forEach(listener => this.state.subscription.listeners.subscribe(listener)) + }) } const childPropsSelector = this.createChildSelector() diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index db8146799..05ef0abf7 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -44,6 +44,11 @@ function createListenerCollection() { } } +// subscriptions are set up in componentDidMount, so we need to delay the actual subscribing until then. +// To do this, the subscription object is created but does not subscribe until the parent is ready +// in cDM, a parent calls "hydrate()" which triggers the resolution of the promise. If +// a child component tries to subscribe before this point, it will wait for the parent subscription +// to be ready and then try to subscribe. export default class Subscription { constructor(store, parentSub, onStateChange) { this.store = store @@ -51,6 +56,25 @@ export default class Subscription { this.onStateChange = onStateChange this.unsubscribe = null this.listeners = nullListeners + this.loaded = false + const resolve = resolve => { + this.resolve = () => { + this.loaded = true + resolve() + } + } + this.ready = new Promise(resolve) + } + + hydrate() { + if (!this.loaded) { + this.resolve() + } + this.trySubscribe() + } + + isReady() { + return this.loaded } addNestedSub(listener) { @@ -66,13 +90,24 @@ export default class Subscription { return Boolean(this.unsubscribe) } + subscribeToParent() { + this.unsubscribe = this.parentSub.addNestedSub(this.onStateChange) + this.listeners = createListenerCollection() + } + trySubscribe() { if (!this.unsubscribe) { - this.unsubscribe = this.parentSub - ? this.parentSub.addNestedSub(this.onStateChange) - : this.store.subscribe(this.onStateChange) - - this.listeners = createListenerCollection() + if (this.parentSub) { + if (this.parentSub.isReady()) { + return this.subscribeToParent() + } + this.parentSub.ready.then(() => { + this.subscribeToParent() + }) + } else { + this.unsubscribe = this.store.subscribe(this.onStateChange) + this.listeners = createListenerCollection() + } } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index c80fffa11..086ccae74 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2310,11 +2310,13 @@ describe('React', () => { expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(1) + console.log('DISPATCH') store1.dispatch({ type: 'INC' }) expect(mapStateToPropsB).toHaveBeenCalledTimes(1) expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(2) + console.log('DISPATCH 2') store2.dispatch({ type: 'INC' }) expect(mapStateToPropsB).toHaveBeenCalledTimes(2) expect(mapStateToPropsC).toHaveBeenCalledTimes(2) From 8a8e15104fb989b0ba4c9531b4e72d1cec6da21a Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Thu, 2 Aug 2018 15:53:17 -0400 Subject: [PATCH 12/19] update subscription change check to be accurate This needs to check whether notifyNestedSubs has changed, not subscription, now that subscription is set in the constructor and only changes its internals when it is safe. --- src/components/connectAdvanced.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index dead254c4..82e2b82cd 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -119,7 +119,7 @@ export default function connectAdvanced( storeState, error: null, subscription: new Subscription(store, subscription, this.updateChildPropsFromReduxStore.bind(this)), - lastSub: null, + lastNotify: noop, notifyNestedSubs: noop } this.state = { @@ -140,8 +140,8 @@ export default function connectAdvanced( static getDerivedStateFromProps(props, state) { let ret = null - if (state.lastSub !== state.subscription) { - ret = { lastSub: state.subscription } + if (state.lastNotify !== state.notifyNestedSubs) { + ret = { lastNotify: state.notifyNestedSubs } if (shallowEqual(props, state.props) || state.error) { return ret } @@ -149,9 +149,9 @@ export default function connectAdvanced( if ((connectOptions.pure && shallowEqual(props, state.props)) || state.error) return ret const nextChildProps = Connect.getChildPropsState(props, state) return { + ...ret, ...nextChildProps, - props, - lastSub: state.subscription + props } } @@ -179,7 +179,6 @@ export default function connectAdvanced( if (this.state.subscription) this.state.subscription.tryUnsubscribe() this.isUnmounted = true this.setState({ - subscription: null, store: null, notifyNestedSubs: noop }) @@ -194,7 +193,6 @@ export default function connectAdvanced( // connected to the store via props shouldn't use subscription from context, or vice versa. this.state.subscription.hydrate() return { - lastSub: state.subscription, // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `this.state.subscription` will then be null. An @@ -202,7 +200,8 @@ export default function connectAdvanced( // replacing it with a no-op on unmount. This can probably be avoided if Subscription's // listeners logic is changed to not call listeners that have been unsubscribed in the // middle of the notification loop. - notifyNestedSubs: this.state.subscription.notifyNestedSubs.bind(this.state.subscription) + notifyNestedSubs: state.subscription.notifyNestedSubs.bind(this.state.subscription), + lastNotify: state.notifyNestedSubs } }, () => { if (hotReloadCallback) { From 52d4f45bc53ec4c75168d07eec18012adff7d7ed Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Thu, 2 Aug 2018 21:36:24 -0400 Subject: [PATCH 13/19] rename resolves to be clearer --- src/utils/Subscription.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index 05ef0abf7..56faa29c4 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -57,18 +57,18 @@ export default class Subscription { this.unsubscribe = null this.listeners = nullListeners this.loaded = false - const resolve = resolve => { - this.resolve = () => { + const cb = resolve => { + this.markReady = () => { this.loaded = true resolve() } } - this.ready = new Promise(resolve) + this.ready = new Promise(cb) } hydrate() { if (!this.loaded) { - this.resolve() + this.markReady() } this.trySubscribe() } From 33333634b99ebc219f08c78d73569317b3ac03cd Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Thu, 2 Aug 2018 22:37:07 -0400 Subject: [PATCH 14/19] resolve conflicts --- package-lock.json | 86 +++++++++++---------------------- package.json | 6 +-- test/components/connect.spec.js | 2 +- 3 files changed, 32 insertions(+), 62 deletions(-) diff --git a/package-lock.json b/package-lock.json index 347944139..04adcc755 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3161,8 +3161,7 @@ "ansi-regex": { "version": "2.1.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "aproba": { "version": "1.1.1", @@ -3228,7 +3227,6 @@ "version": "0.0.9", "bundled": true, "dev": true, - "optional": true, "requires": { "inherits": "~2.0.0" } @@ -3237,7 +3235,6 @@ "version": "2.10.1", "bundled": true, "dev": true, - "optional": true, "requires": { "hoek": "2.x.x" } @@ -3254,8 +3251,7 @@ "buffer-shims": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "caseless": { "version": "0.12.0", @@ -3272,14 +3268,12 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "combined-stream": { "version": "1.0.5", "bundled": true, "dev": true, - "optional": true, "requires": { "delayed-stream": "~1.0.0" } @@ -3292,20 +3286,17 @@ "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "core-util-is": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "cryptiles": { "version": "2.0.5", "bundled": true, "dev": true, - "optional": true, "requires": { "boom": "2.x.x" } @@ -3345,8 +3336,7 @@ "delayed-stream": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "delegates": { "version": "1.0.0", @@ -3378,8 +3368,7 @@ "extsprintf": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "forever-agent": { "version": "0.6.1", @@ -3407,7 +3396,6 @@ "version": "1.0.11", "bundled": true, "dev": true, - "optional": true, "requires": { "graceful-fs": "^4.1.2", "inherits": "~2.0.0", @@ -3475,8 +3463,7 @@ "graceful-fs": { "version": "4.1.11", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "har-schema": { "version": "1.0.5", @@ -3504,7 +3491,6 @@ "version": "3.1.3", "bundled": true, "dev": true, - "optional": true, "requires": { "boom": "2.x.x", "cryptiles": "2.x.x", @@ -3552,7 +3538,6 @@ "version": "1.0.0", "bundled": true, "dev": true, - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -3566,8 +3551,7 @@ "isarray": { "version": "1.0.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "isstream": { "version": "0.1.2", @@ -3640,14 +3624,12 @@ "mime-db": { "version": "1.27.0", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "mime-types": { "version": "2.1.15", "bundled": true, "dev": true, - "optional": true, "requires": { "mime-db": "~1.27.0" } @@ -3663,14 +3645,12 @@ "minimist": { "version": "0.0.8", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "mkdirp": { "version": "0.5.1", "bundled": true, "dev": true, - "optional": true, "requires": { "minimist": "0.0.8" } @@ -3725,8 +3705,7 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "oauth-sign": { "version": "0.8.2", @@ -3784,8 +3763,7 @@ "process-nextick-args": { "version": "1.0.7", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "punycode": { "version": "1.4.1", @@ -3823,7 +3801,6 @@ "version": "2.2.9", "bundled": true, "dev": true, - "optional": true, "requires": { "buffer-shims": "~1.0.0", "core-util-is": "~1.0.0", @@ -3875,8 +3852,7 @@ "safe-buffer": { "version": "5.0.1", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "semver": { "version": "5.3.0", @@ -3900,7 +3876,6 @@ "version": "1.0.9", "bundled": true, "dev": true, - "optional": true, "requires": { "hoek": "2.x.x" } @@ -3934,7 +3909,6 @@ "version": "1.0.2", "bundled": true, "dev": true, - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -3945,7 +3919,6 @@ "version": "1.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "safe-buffer": "^5.0.1" } @@ -3960,7 +3933,6 @@ "version": "3.0.1", "bundled": true, "dev": true, - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -3975,7 +3947,6 @@ "version": "2.2.1", "bundled": true, "dev": true, - "optional": true, "requires": { "block-stream": "*", "fstream": "^1.0.2", @@ -4031,8 +4002,7 @@ "util-deprecate": { "version": "1.0.2", "bundled": true, - "dev": true, - "optional": true + "dev": true }, "uuid": { "version": "3.0.1", @@ -7021,9 +6991,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.2", + "resolved": "https://registry.npmjs.org/react/-/react-16.4.2.tgz", + "integrity": "sha512-dMv7YrbxO4y2aqnvA7f/ik9ibeLSHQJTI6TrYAenPSaQ6OXfb+Oti+oJiy8WBxgRzlKatYqtCjphTgDSCEiWFg==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -7033,9 +7003,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.2", + "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-16.4.2.tgz", + "integrity": "sha512-Usl73nQqzvmJN+89r97zmeUpQDKDlh58eX6Hbs/ERdDHzeBzWy+ENk7fsGQ+5KxArV1iOFPT46/VneklK9zoWw==", "dev": true, "requires": { "fbjs": "^0.8.16", @@ -7045,9 +7015,9 @@ } }, "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==", + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.4.2.tgz", + "integrity": "sha512-rI3cGFj/obHbBz156PvErrS5xc6f1eWyTwyV4mo0vF2lGgXgS+mm7EKD5buLJq6jNgIagQescGSVG2YzgXt8Yg==", "dev": true }, "react-lifecycles-compat": { @@ -7068,15 +7038,15 @@ } }, "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.2", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.2.tgz", + "integrity": "sha512-vdTPnRMDbxfv4wL4lzN4EkVGXyYs7LE2uImOsqh1FKiP6L5o1oJl8nore5sFi9vxrP9PK3l4rgb/fZ4PVUaWSA==", "dev": true, "requires": { "fbjs": "^0.8.16", "object-assign": "^4.1.1", "prop-types": "^15.6.0", - "react-is": "^16.3.2" + "react-is": "^16.4.2" } }, "read-pkg": { diff --git a/package.json b/package.json index 2237fee36..da28d86db 100644 --- a/package.json +++ b/package.json @@ -90,9 +90,9 @@ "glob": "^7.1.1", "jest": "^23.4.1", "npm-run": "^5.0.1", - "react": "^16.3.2", - "react-dom": "^16.3.2", - "react-test-renderer": "^16.3.2", + "react": "^16.4.2", + "react-dom": "^16.4.2", + "react-test-renderer": "^16.4.2", "redux": "^4.0.0", "rimraf": "^2.6.2", "rollup": "^0.61.1", diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 9d2c82822..8352e0f7e 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1881,7 +1881,7 @@ describe('React', () => { store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(4) expect(renderCalls).toBe(2) - expect(spy).toHaveBeenCalledTimes(3) + expect(spy).toHaveBeenCalledTimes(5) // 2 times are subscription setting spy.mockRestore() }) From 1d8ab34955a8bd338cd70ce53fc7a02eeec503fe Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Thu, 2 Aug 2018 22:48:15 -0400 Subject: [PATCH 15/19] remove console statements --- test/components/connect.spec.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 06e26c8d1..0576f313a 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2325,13 +2325,11 @@ describe('React', () => { expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(1) - console.log('DISPATCH') store1.dispatch({ type: 'INC' }) expect(mapStateToPropsB).toHaveBeenCalledTimes(1) expect(mapStateToPropsC).toHaveBeenCalledTimes(1) expect(mapStateToPropsD).toHaveBeenCalledTimes(2) - console.log('DISPATCH 2') store2.dispatch({ type: 'INC' }) expect(mapStateToPropsB).toHaveBeenCalledTimes(2) expect(mapStateToPropsC).toHaveBeenCalledTimes(2) From fe60a4bbb618a2406aa21bfa2d983eb3bcb18a97 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Fri, 3 Aug 2018 10:34:06 -0400 Subject: [PATCH 16/19] fix for React 16-16.3 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- --- src/components/connectAdvanced.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 82e2b82cd..092495d0c 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,6 +1,6 @@ import hoistStatics from 'hoist-non-react-statics' import invariant from 'invariant' -import { Component, createElement } from 'react' +import React, { Component, createElement } from 'react' import { polyfill } from 'react-lifecycles-compat' import shallowEqual from '../utils/shallowEqual' @@ -10,6 +10,12 @@ import { storeShape, subscriptionShape } from '../utils/PropTypes' let hotReloadingVersion = 0 function noop() {} +function isOldReact() { + const version = React.version.split('.') + if (+version[0] < 16) return true + if (+version[0] > 16) return false + return +version[1] < 4 +} export default function connectAdvanced( /* selectorFactory is a func that is responsible for returning the selector function used to @@ -140,7 +146,11 @@ export default function connectAdvanced( static getDerivedStateFromProps(props, state) { let ret = null - if (state.lastNotify !== state.notifyNestedSubs) { + // this next check only should trigger if gDSFP reacts to state changes + // 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) { ret = { lastNotify: state.notifyNestedSubs } if (shallowEqual(props, state.props) || state.error) { return ret From 72fca1c74be0fb03612938a888dfa0796b01fad4 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Fri, 3 Aug 2018 11:13:26 -0400 Subject: [PATCH 17/19] fix breaking tests Also improves performance it seemed componentDidUpdate was needed to ensure subscriptions work, but this is untrue, and causes infinite loops on pre-React 16 --- package-lock.json | 14 ++++++++++++++ src/components/connectAdvanced.js | 4 ---- test/components/connect.spec.js | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 04adcc755..1be86e993 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2441,6 +2441,20 @@ "prop-types": "^15.6.0", "react-reconciler": "^0.7.0", "react-test-renderer": "^16.0.0-0" + }, + "dependencies": { + "react-test-renderer": { + "version": "16.4.2", + "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.4.2.tgz", + "integrity": "sha512-vdTPnRMDbxfv4wL4lzN4EkVGXyYs7LE2uImOsqh1FKiP6L5o1oJl8nore5sFi9vxrP9PK3l4rgb/fZ4PVUaWSA==", + "dev": true, + "requires": { + "fbjs": "^0.8.16", + "object-assign": "^4.1.1", + "prop-types": "^15.6.0", + "react-is": "^16.4.2" + } + } } }, "enzyme-adapter-utils": { diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 092495d0c..99f7dff8b 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -333,12 +333,8 @@ export default function connectAdvanced( const childPropsSelector = this.createChildSelector() const childProps = childPropsSelector(this.props, this.state.storeState) this.setState({ childPropsSelector, childProps }) - } else { - this.updateSubscription() } } - } else { - Connect.prototype.componentDidUpdate = Connect.prototype.updateSubscription } return hoistStatics(Connect, WrappedComponent) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 0576f313a..f023a4b9c 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1881,7 +1881,7 @@ describe('React', () => { store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(4) expect(renderCalls).toBe(2) - expect(spy).toHaveBeenCalledTimes(5) // 2 times are subscription setting + expect(spy).toHaveBeenCalledTimes(3) spy.mockRestore() }) From 1285e4e0c24a8c4bdc29d46751a538686f3f0266 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Fri, 3 Aug 2018 11:19:24 -0400 Subject: [PATCH 18/19] remove unnecessary Promise (React lifecycle does this for us) --- src/utils/Subscription.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/utils/Subscription.js b/src/utils/Subscription.js index 56faa29c4..24635f60f 100644 --- a/src/utils/Subscription.js +++ b/src/utils/Subscription.js @@ -57,13 +57,10 @@ export default class Subscription { this.unsubscribe = null this.listeners = nullListeners this.loaded = false - const cb = resolve => { - this.markReady = () => { - this.loaded = true - resolve() - } - } - this.ready = new Promise(cb) + } + + markReady() { + this.loaded = true } hydrate() { @@ -101,9 +98,6 @@ export default class Subscription { if (this.parentSub.isReady()) { return this.subscribeToParent() } - this.parentSub.ready.then(() => { - this.subscribeToParent() - }) } else { this.unsubscribe = this.store.subscribe(this.onStateChange) this.listeners = createListenerCollection() From 35e9d12b32ff13c8c5cec62422528ccc14dbf300 Mon Sep 17 00:00:00 2001 From: Gregory Beaver Date: Fri, 3 Aug 2018 22:24:51 -0400 Subject: [PATCH 19/19] fix nitpicks and use a constant instead of function call every connect() --- src/components/connectAdvanced.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 99f7dff8b..f07175dd7 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -16,6 +16,8 @@ function isOldReact() { if (+version[0] > 16) return false return +version[1] < 4 } +const oldReact = isOldReact() + export default function connectAdvanced( /* selectorFactory is a func that is responsible for returning the selector function used to @@ -149,8 +151,8 @@ export default function connectAdvanced( // this next check only should trigger if gDSFP reacts to state changes // 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) { + // the oldReact check must be removed + if (!oldReact && state.lastNotify !== state.notifyNestedSubs) { ret = { lastNotify: state.notifyNestedSubs } if (shallowEqual(props, state.props) || state.error) { return ret @@ -199,11 +201,8 @@ export default function connectAdvanced( this.setState(state => { 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() return { - // `notifyNestedSubs` is duplicated to handle the case where the component is unmounted in // the middle of the notification loop, where `this.state.subscription` will then be null. An // extra null check every change can be avoided by copying the method onto `this` and then @@ -217,12 +216,6 @@ export default function connectAdvanced( if (hotReloadCallback) { hotReloadCallback() } - // componentWillMount fires during server side rendering, but componentDidMount and - // componentWillUnmount do not. Because of this, trySubscribe happens during ...didMount. - // Otherwise, unsubscription would never take place during SSR, causing a memory leak. - // 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) }) }