Remove usages of async-unsafe lifecycle methods (reduxjs#919)

* Reformat to make VS Code less annoying

* Failing test in <StrictMode>

* 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

Author: Hypnosphi

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'`
 

package-lock.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",

package.json

@@ -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()
         }
       }

src/components/Provider.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,25 +159,19 @@ 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() {
         if (this.subscription) this.subscription.tryUnsubscribe()
         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)
   }
 }

src/components/connectAdvanced.js

@@ -220,4 +220,19 @@ describe('React', () => {
     store.dispatch({ type: 'APPEND', body: 'd' })
     expect(childMapStateInvokes).toBe(4)
   })
+
+  it('works in <StrictMode> without warnings', () => {
+    const spy = jest.spyOn(console, 'error').mockImplementation(() => {})
+    const store = createStore(() => ({}))
+
+    TestRenderer.create(
+      <React.StrictMode>
+        <Provider store={store}>
+          <div />
+        </Provider>
+      </React.StrictMode>
+    )
+
+    expect(spy).not.toHaveBeenCalled()
+  })
 })