From 2a3c22ebb88a1e54a7a8776d4cc4b6dec31761be Mon Sep 17 00:00:00 2001 From: Filipp Riabchun Date: Thu, 21 Jun 2018 19:33:28 +0300 Subject: [PATCH] Remove usages of async-unsafe lifecycle methods (#919) * Reformat to make VS Code less annoying * Failing test in * Remove usages of async-unsafe lifecycle methods * Remove usage of UNSAFE_componentWillMount in test * Move `selector` to state in order to be able to use gDSFP * codeDIdCommit * Stop using mutation * Upgrade react-lifecycles-compat --- docs/api.md | 2 +- package-lock.json | 11 +++ package.json | 3 +- src/components/Provider.js | 4 +- src/components/connectAdvanced.js | 108 ++++++++++++++---------------- test/components/Provider.spec.js | 15 +++++ test/components/connect.spec.js | 58 ++++++++++------ 7 files changed, 120 insertions(+), 81 deletions(-) diff --git a/docs/api.md b/docs/api.md index e4524bf0d..712216bc8 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/package-lock.json b/package-lock.json index c76413e43..8c0b495a9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6881,6 +6881,17 @@ "prop-types": "15.6.1" } }, + "react-is": { + "version": "16.3.0", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.3.0.tgz", + "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.2", "resolved": "https://registry.npmjs.org/react-test-renderer/-/react-test-renderer-16.3.2.tgz", diff --git a/package.json b/package.json index fdc6c5ed6..ed9a8dd4d 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,8 @@ "hoist-non-react-statics": "^2.5.0", "invariant": "^2.2.4", "loose-envify": "^1.1.0", - "prop-types": "^15.6.1" + "prop-types": "^15.6.1", + "react-lifecycles-compat": "^3.0.0" }, "devDependencies": { "babel-cli": "^6.26.0", diff --git a/src/components/Provider.js b/src/components/Provider.js index f78e25e65..a0d6e3b0c 100644 --- a/src/components/Provider.js +++ b/src/components/Provider.js @@ -38,8 +38,8 @@ export function createProvider(storeKey = 'store') { } 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..1a5faa625 100644 --- a/src/components/connectAdvanced.js +++ b/src/components/connectAdvanced.js @@ -1,32 +1,34 @@ 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 -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 + } + return { + shouldComponentUpdate: false, + } + } catch (error) { + return { + shouldComponentUpdate: true, + error, } } } - - return selector } export default function connectAdvanced( @@ -86,6 +88,10 @@ export default function connectAdvanced( [subscriptionKey]: subscriptionShape, } + function getDerivedStateFromProps(nextProps, prevState) { + return prevState.updater(nextProps, prevState) + } + return function wrapWithConnect(WrappedComponent) { invariant( typeof WrappedComponent == 'function', @@ -117,7 +123,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 +134,9 @@ export default function connectAdvanced( `or explicitly pass "${storeKey}" as a prop to "${displayName}".` ) - this.initSelector() + this.state = { + updater: this.createUpdater() + } this.initSubscription() } @@ -152,16 +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.selector.run(this.props) - if (this.selector.shouldComponentUpdate) this.forceUpdate() - } - - componentWillReceiveProps(nextProps) { - this.selector.run(nextProps) + this.runUpdater() } - shouldComponentUpdate() { - return this.selector.shouldComponentUpdate + shouldComponentUpdate(_, nextState) { + return nextState.shouldComponentUpdate } componentWillUnmount() { @@ -169,8 +171,7 @@ export default function connectAdvanced( this.subscription = null this.notifyNestedSubs = noop this.store = null - this.selector.run = noop - this.selector.shouldComponentUpdate = false + this.isUnmounted = true } getWrappedInstance() { @@ -185,10 +186,17 @@ export default function connectAdvanced( this.wrappedInstance = ref } - initSelector() { + createUpdater() { const sourceSelector = selectorFactory(this.store.dispatch, selectorFactoryOptions) - this.selector = makeSelectorStateful(sourceSelector, this.store) - this.selector.run(this.props) + return makeUpdater(sourceSelector, this.store) + } + + runUpdater(callback = noop) { + if (this.isUnmounted) { + return + } + + this.setState(prevState => prevState.updater(this.props, prevState), callback) } initSubscription() { @@ -209,24 +217,7 @@ export default function connectAdvanced( } onStateChange() { - this.selector.run(this.props) - - if (!this.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() { @@ -247,13 +238,10 @@ export default function connectAdvanced( } render() { - const selector = this.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)) } } } @@ -263,13 +251,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.componentWillUpdate = function componentWillUpdate() { + 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 @@ -287,10 +275,16 @@ export default function connectAdvanced( this.subscription.trySubscribe() oldListeners.forEach(listener => this.subscription.listeners.subscribe(listener)) } + + const updater = this.createUpdater() + this.setState({updater}) + this.runUpdater() } } } + polyfill(Connect) + return hoistStatics(Connect, WrappedComponent) } } diff --git a/test/components/Provider.spec.js b/test/components/Provider.spec.js index 6830b8f22..bd3d2af1c 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 06951c17f..63e8d9368 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' }); } } @@ -1045,7 +1045,7 @@ describe('React', () => { spy.mockRestore() document.body.removeChild(div) - expect(mapStateToPropsCalls).toBe(3) + expect(mapStateToPropsCalls).toBe(2) expect(spy).toHaveBeenCalledTimes(0) }) @@ -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() { } @@ -1857,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() }) @@ -1928,7 +1927,7 @@ describe('React', () => { @connect(mapStateFactory) class Container extends Component { - componentWillUpdate() { + componentDidUpdate() { updatedCount++ } render() { @@ -2009,7 +2008,7 @@ describe('React', () => { @connect(null, mapDispatchFactory, mergeParentDispatch) class Passthrough extends Component { - componentWillUpdate() { + componentDidUpdate() { updatedCount++ } render() { @@ -2121,10 +2120,6 @@ describe('React', () => { @connect(null) class Parent extends React.Component { - componentWillMount() { - this.props.dispatch({ type: 'fetch' }) - } - componentWillUnmount() { this.props.dispatch({ type: 'clean' }) } @@ -2144,6 +2139,7 @@ describe('React', () => { } const store = createStore(reducer) + store.dispatch({ type: 'fetch' }) const div = document.createElement('div') ReactDOM.render( @@ -2325,6 +2321,28 @@ describe('React', () => { 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() + }) + it('should receive the store in the context using a custom store key', () => { const store = createStore(() => ({})) const CustomProvider = createProvider('customStoreKey')