From 7b4e1626ac210ae539080222faf5d75cff80ecd0 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Mon, 2 Apr 2018 12:58:49 -0400 Subject: [PATCH 1/8] Reformat to make VS Code less annoying --- test/components/connect.spec.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 2d3e975ce..6540ab133 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1218,14 +1218,13 @@ describe('React', () => { const store = createStore(() => ({})) function makeContainer(mapState, mapDispatch, mergeProps) { - return React.createElement( - @connect(mapState, mapDispatch, mergeProps) - class Container extends Component { - render() { - return - } + @connect(mapState, mapDispatch, mergeProps) + class Container extends Component { + render() { + return } - ) + } + return React.createElement(Container) } function AwesomeMap() { } From 6ad378b8cd678e38878ccc37627d205f0a11f4b0 Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Mon, 2 Apr 2018 13:50:57 -0400 Subject: [PATCH 2/8] Failing test in --- test/components/connect.spec.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 6540ab133..06dbce7df 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2323,5 +2323,27 @@ describe('React', () => { expect(mapStateToPropsC).toHaveBeenCalledTimes(2) expect(mapStateToPropsD).toHaveBeenCalledTimes(2) }) + + it('works in without warnings', () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + const store = createStore(stringBuilder) + + @connect(state => ({ string: state }) ) + class Container extends Component { + render() { + return + } + } + + TestRenderer.create( + + + + + + ) + + expect(spy).not.toHaveBeenCalled() + }) }) }) From 20de413e456f7426521b898b38839e287b3b1451 Mon Sep 17 00:00:00 2001 From: Hypnosphi Date: Tue, 3 Apr 2018 03:56:30 +0300 Subject: [PATCH 3/8] Remove usages of async-unsafe lifecycle methods --- docs/api.md | 2 +- src/components/Provider.js | 4 ++-- src/components/connectAdvanced.js | 17 +++++++++++------ test/components/Provider.spec.js | 15 +++++++++++++++ test/components/connect.spec.js | 12 ++++++------ 5 files changed, 35 insertions(+), 15 deletions(-) diff --git a/docs/api.md b/docs/api.md index 93c231bf4..914ff3d20 100644 --- a/docs/api.md +++ b/docs/api.md @@ -377,7 +377,7 @@ It does not modify the component class passed to it; instead, it *returns* a new * [`renderCountProp`] *(String)*: if defined, a property named this value will be added to the props passed to the wrapped component. Its value will be the number of times the component has been rendered, which can be useful for tracking down unnecessary re-renders. Default value: `undefined` - * [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render on `componentWillReceiveProps`. Default value: `true` + * [`shouldHandleStateChanges`] *(Boolean)*: controls whether the connector component subscribes to redux store state changes. If set to false, it will only re-render when parent component re-renders. Default value: `true` * [`storeKey`] *(String)*: the key of props/context to get the store. You probably only need this if you are in the inadvisable position of having multiple stores. Default value: `'store'` diff --git a/src/components/Provider.js b/src/components/Provider.js index ac77a003e..098a88a28 100644 --- a/src/components/Provider.js +++ b/src/components/Provider.js @@ -38,8 +38,8 @@ export function createProvider(storeKey = 'store', subKey) { } if (process.env.NODE_ENV !== 'production') { - Provider.prototype.componentWillReceiveProps = function (nextProps) { - if (this[storeKey] !== nextProps.store) { + Provider.prototype.componentDidUpdate = function () { + if (this[storeKey] !== this.props.store) { warnAboutReceivingStore() } } diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 3fff8ee26..0720d5f75 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -156,11 +156,10 @@ export default function connectAdvanced( if (this.selector.shouldComponentUpdate) this.forceUpdate() } - componentWillReceiveProps(nextProps) { - this.selector.run(nextProps) - } - - shouldComponentUpdate() { + shouldComponentUpdate(nextProps) { + if (nextProps !== this.props) { + this.selector.run(nextProps) + } return this.selector.shouldComponentUpdate } @@ -248,6 +247,10 @@ export default function connectAdvanced( render() { const selector = this.selector + + // Handle forceUpdate + if (!selector.shouldComponentUpdate) selector.run(this.props) + selector.shouldComponentUpdate = false if (selector.error) { @@ -265,7 +268,7 @@ export default function connectAdvanced( Connect.propTypes = contextTypes if (process.env.NODE_ENV !== 'production') { - Connect.prototype.componentWillUpdate = function componentWillUpdate() { + Connect.prototype.componentDidUpdate = function componentDidUpdate() { // We are hot reloading! if (this.version !== version) { this.version = version @@ -287,6 +290,8 @@ export default function connectAdvanced( this.subscription.trySubscribe() oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) } + + if (this.selector.shouldComponentUpdate) this.setState(dummyState) } } } diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index 449cdf0fb..924015929 100644 --- a/test/components/Provider.spec.js +++ b/test/components/Provider.spec.js @@ -220,4 +220,19 @@ describe('React', () => { store.dispatch({ type: 'APPEND', body: 'd' }) expect(childMapStateInvokes).toBe(4) }) + + it('works in without warnings', () => { + const spy = jest.spyOn(console, 'error').mockImplementation(() => {}) + const store = createStore(() => ({})) + + TestRenderer.create( + + +
+ + + ) + + expect(spy).not.toHaveBeenCalled() + }) }) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 06dbce7df..3cdb71952 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -200,7 +200,7 @@ describe('React', () => { @connect(state => ({ string: state }) ) class Container extends Component { - componentWillMount() { + componentDidMount() { store.dispatch({ type: 'APPEND', body: 'a' }) } @@ -944,8 +944,8 @@ describe('React', () => { @connect((state) => ({ state })) class Child extends Component { - componentWillReceiveProps(nextProps) { - if (nextProps.state === 'A') { + componentDidMount() { + if (this.props.state === 'A') { store.dispatch({ type: 'APPEND', body: 'B' }); } } @@ -1927,7 +1927,7 @@ describe('React', () => { @connect(mapStateFactory) class Container extends Component { - componentWillUpdate() { + componentDidUpdate() { updatedCount++ } render() { @@ -2008,7 +2008,7 @@ describe('React', () => { @connect(null, mapDispatchFactory, mergeParentDispatch) class Passthrough extends Component { - componentWillUpdate() { + componentDIdUpdate() { updatedCount++ } render() { @@ -2120,7 +2120,7 @@ describe('React', () => { @connect(null) class Parent extends React.Component { - componentWillMount() { + UNSAFE_componentWillMount() { this.props.dispatch({ type: 'fetch' }) } From 94f0b8a7540a2acdf2f121c5e33573fdf7727f09 Mon Sep 17 00:00:00 2001 From: Hypnosphi Date: Wed, 4 Apr 2018 02:42:02 +0300 Subject: [PATCH 4/8] Remove usage of UNSAFE_componentWillMount in test --- test/components/connect.spec.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 3cdb71952..a2d9bdfd5 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2120,10 +2120,6 @@ describe('React', () => { @connect(null) class Parent extends React.Component { - UNSAFE_componentWillMount() { - this.props.dispatch({ type: 'fetch' }) - } - componentWillUnmount() { this.props.dispatch({ type: 'clean' }) } @@ -2143,6 +2139,7 @@ describe('React', () => { } const store = createStore(reducer) + store.dispatch({ type: 'fetch' }) const div = document.createElement('div') ReactDOM.render( From 9ee1a360b66226bb6380bf07abb53085dc89ee87 Mon Sep 17 00:00:00 2001 From: Hypnosphi Date: Wed, 4 Apr 2018 02:45:43 +0300 Subject: [PATCH 5/8] Move `selector` to state in order to be able to use gDSFP --- package-lock.json | 5 +++ package.json | 3 +- src/components/connectAdvanced.js | 51 ++++++++++++++++++------------- 3 files changed, 36 insertions(+), 23 deletions(-) diff --git a/package-lock.json b/package-lock.json index b394d8024..596d6d4f4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6769,6 +6769,11 @@ "integrity": "sha512-YOo+BNK2z8LiDxh2viaOklPqhwrMMsNPWnXTseOqJa8/ob8mv9aD9Z5FqqQnKNbIerBm+DoIwjAAAKcpdDo1/w==", "dev": true }, + "react-lifecycles-compat": { + "version": "1.1.4", + "resolved": "https://registry.npmjs.org/react-lifecycles-compat/-/react-lifecycles-compat-1.1.4.tgz", + "integrity": "sha512-g3pdexIqkn+CVvSpYIoyON8zUbF9kgfhp672gyz7wQ7PQyXVmJtah+GDYqpHpOrdwex3F77iv+alq79iux9HZw==" + }, "react-test-renderer": { "version": "16.3.0", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.3.0.tgz", diff --git a/package.json b/package.json index d6eb0e33d..fb5123f89 100644 --- a/package.json +++ b/package.json @@ -53,7 +53,8 @@ "lodash": "^4.17.5", "lodash-es": "^4.17.5", "loose-envify": "^1.1.0", - "prop-types": "^15.6.1" + "prop-types": "^15.6.1", + "react-lifecycles-compat": "^1.1.4" }, "devDependencies": { "babel-cli": "^6.26.0", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 0720d5f75..691564582 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 Subscription from '../utils/Subscription' import { storeShape, subscriptionShape } from '../utils/PropTypes' @@ -23,6 +24,10 @@ function makeSelectorStateful(sourceSelector, store) { selector.shouldComponentUpdate = true selector.error = error } + }, + clean: function cleanComponentSelector() { + selector.run = noop + selector.shouldComponentUpdate = false } } @@ -86,6 +91,11 @@ export default function connectAdvanced( [subscriptionKey]: subscriptionShape, } + function getDerivedStateFromProps(nextProps, prevState) { + prevState.selector.run(nextProps) + return null + } + return function wrapWithConnect(WrappedComponent) { invariant( typeof WrappedComponent == 'function', @@ -117,7 +127,6 @@ export default function connectAdvanced( super(props, context) this.version = version - this.state = {} this.renderCount = 0 this.store = props[storeKey] || context[storeKey] this.propsMode = Boolean(props[storeKey]) @@ -129,7 +138,9 @@ export default function connectAdvanced( `or explicitly pass "${storeKey}" as a prop to "${displayName}".` ) - this.initSelector() + this.state = { + selector: this.createSelector() + } this.initSubscription() } @@ -152,15 +163,12 @@ 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.selector.run(this.props) - if (this.selector.shouldComponentUpdate) this.forceUpdate() + this.state.selector.run(this.props) + if (this.state.selector.shouldComponentUpdate) this.forceUpdate() } - shouldComponentUpdate(nextProps) { - if (nextProps !== this.props) { - this.selector.run(nextProps) - } - return this.selector.shouldComponentUpdate + shouldComponentUpdate(nextProps, nextState) { + return nextState.selector.shouldComponentUpdate } componentWillUnmount() { @@ -168,8 +176,7 @@ export default function connectAdvanced( this.subscription = null this.notifyNestedSubs = noop this.store = null - this.selector.run = noop - this.selector.shouldComponentUpdate = false + this.state.selector.clean() } getWrappedInstance() { @@ -184,10 +191,9 @@ export default function connectAdvanced( this.wrappedInstance = ref } - initSelector() { + createSelector() { const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions) - this.selector = makeSelectorStateful(sourceSelector, this.store) - this.selector.run(this.props) + return makeSelectorStateful(sourceSelector, this.store) } initSubscription() { @@ -208,9 +214,9 @@ export default function connectAdvanced( } onStateChange() { - this.selector.run(this.props) + this.state.selector.run(this.props) - if (!this.selector.shouldComponentUpdate) { + if (!this.state.selector.shouldComponentUpdate) { this.notifyNestedSubs() } else { this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate @@ -246,10 +252,7 @@ export default function connectAdvanced( } render() { - const selector = this.selector - - // Handle forceUpdate - if (!selector.shouldComponentUpdate) selector.run(this.props) + const selector = this.state.selector selector.shouldComponentUpdate = false @@ -266,13 +269,13 @@ 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() { // We are hot reloading! if (this.version !== version) { this.version = version - this.initSelector() // 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 @@ -291,11 +294,15 @@ export default function connectAdvanced( oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) } - if (this.selector.shouldComponentUpdate) this.setState(dummyState) + const selector = this.createSelector() + selector.run(this.props) + this.setState({selector}) } } } + polyfill(Connect) + return hoistStatics(Connect, WrappedComponent) } } From 7344f3d0aea7da2ddfd3f654a7a7e2b140c5ff6b Mon Sep 17 00:00:00 2001 From: Tim Dorr Date: Thu, 5 Apr 2018 14:44:54 -0400 Subject: [PATCH 6/8] codeDIdCommit --- test/components/connect.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index a2d9bdfd5..0211f3381 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -2008,7 +2008,7 @@ describe('React', () => { @connect(null, mapDispatchFactory, mergeParentDispatch) class Passthrough extends Component { - componentDIdUpdate() { + componentDidUpdate() { updatedCount++ } render() { From a7ba2981b4e4482830da85b105d86600811c7938 Mon Sep 17 00:00:00 2001 From: Hypnosphi Date: Fri, 6 Apr 2018 04:43:46 +0300 Subject: [PATCH 7/8] Stop using mutation --- src/components/connectAdvanced.js | 98 +++++++++++++------------------ test/components/connect.spec.js | 8 +-- 2 files changed, 44 insertions(+), 62 deletions(-) diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index 691564582..ab0d95f7a 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -7,31 +7,28 @@ import Subscription from '../utils/Subscription' import { storeShape, subscriptionShape } from '../utils/PropTypes' let hotReloadingVersion = 0 -const dummyState = {} function noop() {} -function makeSelectorStateful(sourceSelector, store) { - // wrap the selector in an object that tracks its results between runs. - const selector = { - run: function runComponentSelector(props) { - try { - const nextProps = sourceSelector(store.getState(), props) - if (nextProps !== selector.props || selector.error) { - selector.shouldComponentUpdate = true - selector.props = nextProps - selector.error = null +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, } - } catch (error) { - selector.shouldComponentUpdate = true - selector.error = error } - }, - clean: function cleanComponentSelector() { - selector.run = noop - selector.shouldComponentUpdate = false + return { + shouldComponentUpdate: false, + } + } catch (error) { + return { + shouldComponentUpdate: true, + error, + } } } - - return selector } export default function connectAdvanced( @@ -92,8 +89,7 @@ export default function connectAdvanced( } function getDerivedStateFromProps(nextProps, prevState) { - prevState.selector.run(nextProps) - return null + return prevState.updater(nextProps, prevState) } return function wrapWithConnect(WrappedComponent) { @@ -139,7 +135,7 @@ export default function connectAdvanced( ) this.state = { - selector: this.createSelector() + updater: this.createUpdater() } this.initSubscription() } @@ -163,12 +159,11 @@ 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.state.selector.run(this.props) - if (this.state.selector.shouldComponentUpdate) this.forceUpdate() + this.runUpdater() } - shouldComponentUpdate(nextProps, nextState) { - return nextState.selector.shouldComponentUpdate + shouldComponentUpdate(_, nextState) { + return nextState.shouldComponentUpdate } componentWillUnmount() { @@ -176,7 +171,7 @@ export default function connectAdvanced( this.subscription = null this.notifyNestedSubs = noop this.store = null - this.state.selector.clean() + this.isUnmounted = true } getWrappedInstance() { @@ -191,9 +186,17 @@ export default function connectAdvanced( this.wrappedInstance = ref } - createSelector() { + createUpdater() { const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions) - return makeSelectorStateful(sourceSelector, this.store) + return makeUpdater(sourceSelector, this.store) + } + + runUpdater(callback = noop) { + if (this.isUnmounted) { + return + } + + this.setState(prevState => prevState.updater(this.props, prevState), callback) } initSubscription() { @@ -214,24 +217,7 @@ export default function connectAdvanced( } onStateChange() { - this.state.selector.run(this.props) - - if (!this.state.selector.shouldComponentUpdate) { - this.notifyNestedSubs() - } else { - this.componentDidUpdate = this.notifyNestedSubsOnComponentDidUpdate - this.setState(dummyState) - } - } - - notifyNestedSubsOnComponentDidUpdate() { - // `componentDidUpdate` is conditionally implemented when `onStateChange` determines it - // needs to notify nested subs. Once called, it unimplements itself until further state - // changes occur. Doing it this way vs having a permanent `componentDidUpdate` that does - // a boolean check every time avoids an extra method call most of the time, resulting - // in some perf boost. - this.componentDidUpdate = undefined - this.notifyNestedSubs() + this.runUpdater(this.notifyNestedSubs) } isSubscribed() { @@ -252,14 +238,10 @@ export default function connectAdvanced( } render() { - const selector = this.state.selector - - selector.shouldComponentUpdate = false - - if (selector.error) { - throw selector.error + if (this.state.error) { + throw this.state.error } else { - return createElement(WrappedComponent, this.addExtraProps(selector.props)) + return createElement(WrappedComponent, this.addExtraProps(this.state.props)) } } } @@ -294,9 +276,9 @@ export default function connectAdvanced( oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) } - const selector = this.createSelector() - selector.run(this.props) - this.setState({selector}) + const updater = this.createUpdater() + this.setState({updater}) + this.runUpdater() } } } diff --git a/test/components/connect.spec.js b/test/components/connect.spec.js index 0211f3381..c35838a6c 100644 --- a/test/components/connect.spec.js +++ b/test/components/connect.spec.js @@ -1045,7 +1045,7 @@ describe('React', () => { spy.mockRestore() document.body.removeChild(div) - expect(mapStateToPropsCalls).toBe(3) + expect(mapStateToPropsCalls).toBe(2) expect(spy).toHaveBeenCalledTimes(0) }) @@ -1856,17 +1856,17 @@ describe('React', () => { store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(2) expect(renderCalls).toBe(1) - expect(spy).toHaveBeenCalledTimes(0) + expect(spy).toHaveBeenCalledTimes(1) store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(3) expect(renderCalls).toBe(1) - expect(spy).toHaveBeenCalledTimes(0) + expect(spy).toHaveBeenCalledTimes(2) store.dispatch({ type: 'APPEND', body: 'a' }) expect(mapStateCalls).toBe(4) expect(renderCalls).toBe(2) - expect(spy).toHaveBeenCalledTimes(1) + expect(spy).toHaveBeenCalledTimes(3) spy.mockRestore() }) From 27b2d4bc9ae204ee4b1f313980552bf9e007ac62 Mon Sep 17 00:00:00 2001 From: Hypnosphi Date: Tue, 10 Apr 2018 00:50:13 +0300 Subject: [PATCH 8/8] Upgrade react-lifecycles-compat --- package.json | 2 +- src/components/connectAdvanced.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index fb5123f89..6c75e0bbd 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,7 @@ "lodash-es": "^4.17.5", "loose-envify": "^1.1.0", "prop-types": "^15.6.1", - "react-lifecycles-compat": "^1.1.4" + "react-lifecycles-compat": "^3.0.0" }, "devDependencies": { "babel-cli": "^6.26.0", diff --git a/src/components/connectAdvanced.js b/src/components/connectAdvanced.js index ab0d95f7a..1a5faa625 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,7 +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 { polyfill } from 'react-lifecycles-compat' import Subscription from '../utils/Subscription' import { storeShape, subscriptionShape } from '../utils/PropTypes'