chore(AutoControlledComponent): Remove unsafe lifecycles

Remove unsafe lifecycles from `AutoControlledComponent` and affected components: - `componentWillMount` - `componentWillRecieveProps` - `componentWillUpdate` Relates to Semantic-Org#2732
kohakukun · Oct 28, 2018 · babd63d · babd63d
1 parent 6e83af2
commit babd63d
Show file tree

Hide file tree

Showing 12 changed files with 173 additions and 190 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -36,6 +36,8 @@
     "react/jsx-filename-extension": [2, { "extensions": [".js"] }],
     "react/no-unused-prop-types": 0,
     "react/sort-comp": 0,
-    "react/require-default-props": 0
+    "react/require-default-props": 0,
+    "react/no-did-mount-set-state": 0, 
+    "react/no-did-update-set-state": 0
   }
 }
diff --git a/src/lib/AutoControlledComponent.js b/src/lib/AutoControlledComponent.js
@@ -86,11 +86,15 @@ export default class AutoControlledComponent extends Component {
         const defaultProp = getDefaultPropName(prop)
         // regular prop
         if (!_.has(propTypes, defaultProp)) {
-          console.error(`${name} is missing "${defaultProp}" propTypes validation for auto controlled prop "${prop}".`)
+          console.error(
+            `${name} is missing "${defaultProp}" propTypes validation for auto controlled prop "${prop}".`,
+          )
         }
         // its default prop
         if (!_.has(propTypes, prop)) {
-          console.error(`${name} is missing propTypes validation for auto controlled prop "${prop}".`)
+          console.error(
+            `${name} is missing propTypes validation for auto controlled prop "${prop}".`,
+          )
         }
       })
 
@@ -106,25 +110,31 @@ export default class AutoControlledComponent extends Component {
       // https://babeljs.io/blog/2015/06/07/react-on-es6-plus#property-initializers
       const illegalDefaults = _.intersection(autoControlledProps, _.keys(defaultProps))
       if (!_.isEmpty(illegalDefaults)) {
-        console.error([
-          'Do not set defaultProps for autoControlledProps. You can set defaults by',
-          'setting state in the constructor or using an ES7 property initializer',
-          '(https://babeljs.io/blog/2015/06/07/react-on-es6-plus#property-initializers)',
-          `See ${name} props: "${illegalDefaults}".`,
-        ].join(' '))
+        console.error(
+          [
+            'Do not set defaultProps for autoControlledProps. You can set defaults by',
+            'setting state in the constructor or using an ES7 property initializer',
+            '(https://babeljs.io/blog/2015/06/07/react-on-es6-plus#property-initializers)',
+            `See ${name} props: "${illegalDefaults}".`,
+          ].join(' '),
+        )
       }
 
       // prevent listing defaultProps in autoControlledProps
       //
       // Default props are automatically handled.
       // Listing defaults in autoControlledProps would result in allowing defaultDefaultValue props.
-      const illegalAutoControlled = _.filter(autoControlledProps, prop => _.startsWith(prop, 'default'))
+      const illegalAutoControlled = _.filter(autoControlledProps, prop =>
+        _.startsWith(prop, 'default'),
+      )
       if (!_.isEmpty(illegalAutoControlled)) {
-        console.error([
-          'Do not add default props to autoControlledProps.',
-          'Default props are automatically handled.',
-          `See ${name} autoControlledProps: "${illegalAutoControlled}".`,
-        ].join(' '))
+        console.error(
+          [
+            'Do not add default props to autoControlledProps.',
+            'Default props are automatically handled.',
+            `See ${name} autoControlledProps: "${illegalAutoControlled}".`,
+          ].join(' '),
+        )
       }
     }
 
@@ -152,19 +162,21 @@ export default class AutoControlledComponent extends Component {
     this.state = { ...state, ...initialAutoControlledState }
   }
 
-  componentWillReceiveProps(nextProps) {
+  componentDidUpdate(prevProps) {
     const { autoControlledProps } = this.constructor
 
     // Solve the next state for autoControlledProps
     const newState = autoControlledProps.reduce((acc, prop) => {
-      const isNextUndefined = _.isUndefined(nextProps[prop])
-      const propWasRemoved = !_.isUndefined(this.props[prop]) && isNextUndefined
+      const isNextUndefined = _.isUndefined(this.props[prop])
+      const propWasRemoved = !_.isUndefined(prevProps[prop]) && isNextUndefined
 
+      let value
       // if next is defined then use its value
-      if (!isNextUndefined) acc[prop] = nextProps[prop]
-
+      if (!isNextUndefined) value = this.props[prop]
       // reinitialize state for props just removed / set undefined
-      else if (propWasRemoved) acc[prop] = getAutoControlledStateValue(prop, nextProps)
+      else if (propWasRemoved) value = getAutoControlledStateValue(prop, this.props)
+
+      if (_.has(this.props, prop) && this.state[prop] !== value) acc[prop] = value
 
       return acc
     }, {})
@@ -185,11 +197,13 @@ export default class AutoControlledComponent extends Component {
       // warn about failed attempts to setState for keys not listed in autoControlledProps
       const illegalKeys = _.difference(_.keys(maybeState), autoControlledProps)
       if (!_.isEmpty(illegalKeys)) {
-        console.error([
-          `${name} called trySetState() with controlled props: "${illegalKeys}".`,
-          'State will not be set.',
-          'Only props in static autoControlledProps will be set on state.',
-        ].join(' '))
+        console.error(
+          [
+            `${name} called trySetState() with controlled props: "${illegalKeys}".`,
+            'State will not be set.',
+            'Only props in static autoControlledProps will be set on state.',
+          ].join(' '),
+        )
       }
     }
 

diff --git a/src/modules/Accordion/AccordionAccordion.js b/src/modules/Accordion/AccordionAccordion.js
@@ -91,7 +91,8 @@ export default class AccordionAccordion extends Component {
     }
   }
 
-  componentDidUpdate() {
+  componentDidUpdate(prevProps, prevState) {
+    super.componentDidUpdate(prevProps, prevState)
     if (process.env.NODE_ENV !== 'production') {
       warnIfPropsAreInvalid(this.props, this.state)
     }

diff --git a/src/modules/Checkbox/Checkbox.js b/src/modules/Checkbox/Checkbox.js
@@ -113,7 +113,8 @@ export default class Checkbox extends Component {
     this.setIndeterminate()
   }
 
-  componentDidUpdate() {
+  componentDidUpdate(prevProps, prevState) {
+    super.componentDidUpdate(prevProps, prevState)
     this.setIndeterminate()
   }
 

diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js
@@ -382,8 +382,8 @@ export default class Dropdown extends Component {
     return { searchQuery: '' }
   }
 
-  componentWillMount() {
-    debug('componentWillMount()')
+  componentDidMount() {
+    debug('componentDidMount()')
     const { open, value } = this.state
 
     this.setValue(value)
@@ -395,23 +395,29 @@ export default class Dropdown extends Component {
     }
   }
 
-  componentWillReceiveProps(nextProps) {
-    super.componentWillReceiveProps(nextProps)
-    debug('componentWillReceiveProps()')
-    debug('to props:', objectDiff(this.props, nextProps))
+  shouldComponentUpdate(nextProps, nextState) {
+    return !shallowEqual(nextProps, this.props) || !shallowEqual(nextState, this.state)
+  }
+
+  componentDidUpdate(prevProps, prevState) {
+    // eslint-disable-line complexity
+    super.componentDidUpdate(prevProps, prevState)
+    debug('componentDidUpdate()')
+    debug('to state:', objectDiff(prevState, this.state))
+    debug('to props:', objectDiff(prevProps, this.props))
 
     /* eslint-disable no-console */
-    if (process.env.NODE_ENV !== 'production') {
+    if (process.env.NODE_ENV !== 'production' && this.props.value) {
       // in development, validate value type matches dropdown type
-      const isNextValueArray = Array.isArray(nextProps.value)
-      const hasValue = _.has(nextProps, 'value')
+      const isNextValueArray = Array.isArray(this.props.value)
+      const hasValue = _.has(this.props, 'value')
 
-      if (hasValue && nextProps.multiple && !isNextValueArray) {
+      if (hasValue && this.props.multiple && !isNextValueArray) {
         console.error(
           'Dropdown `value` must be an array when `multiple` is set.' +
-            ` Received type: \`${Object.prototype.toString.call(nextProps.value)}\`.`,
+            ` Received type: \`${Object.prototype.toString.call(this.props.value)}\`.`,
         )
-      } else if (hasValue && !nextProps.multiple && isNextValueArray) {
+      } else if (hasValue && !this.props.multiple && isNextValueArray) {
         console.error(
           'Dropdown `value` must not be an array when `multiple` is not set.' +
             ' Either set `multiple={true}` or use a string or number value.',
@@ -420,30 +426,20 @@ export default class Dropdown extends Component {
     }
     /* eslint-enable no-console */
 
-    if (!shallowEqual(nextProps.value, this.props.value)) {
-      debug('value changed, setting', nextProps.value)
-      this.setValue(nextProps.value)
-      this.setSelectedIndex(nextProps.value)
+    if (!shallowEqual(this.props.value, prevProps.value)) {
+      debug('value changed, setting', this.props.value)
+      this.setValue(this.props.value)
+      this.setSelectedIndex(this.props.value)
     }
 
     // The selected index is only dependent on option keys/values.
     // We only check those properties to avoid recursive performance impacts.
     // https://github.com/Semantic-Org/Semantic-UI-React/issues/3000
     if (
-      !_.isEqual(this.getKeyAndValues(nextProps.options), this.getKeyAndValues(this.props.options))
+      !_.isEqual(this.getKeyAndValues(this.props.options), this.getKeyAndValues(prevProps.options))
     ) {
-      this.setSelectedIndex(undefined, nextProps.options)
+      this.setSelectedIndex(undefined, this.props.options)
     }
-  }
-
-  shouldComponentUpdate(nextProps, nextState) {
-    return !shallowEqual(nextProps, this.props) || !shallowEqual(nextState, this.state)
-  }
-
-  componentDidUpdate(prevProps, prevState) {
-    // eslint-disable-line complexity
-    debug('componentDidUpdate()')
-    debug('to state:', objectDiff(prevState, this.state))
 
     // focused / blurred
     if (!prevState.focus && this.state.focus) {

diff --git a/src/modules/Search/Search.js b/src/modules/Search/Search.js
@@ -191,33 +191,29 @@ export default class Search extends Component {
   static Result = SearchResult
   static Results = SearchResults
 
-  componentWillMount() {
-    debug('componentWillMount()')
+  componentDidMount() {
+    debug('componentDidMount()')
     const { open, value } = this.state
 
     this.setValue(value)
     if (open) this.open()
   }
 
-  componentWillReceiveProps(nextProps) {
-    super.componentWillReceiveProps(nextProps)
-    debug('componentWillReceiveProps()')
-    debug('changed props:', objectDiff(nextProps, this.props))
-
-    if (!shallowEqual(nextProps.value, this.props.value)) {
-      debug('value changed, setting', nextProps.value)
-      this.setValue(nextProps.value)
-    }
-  }
-
   shouldComponentUpdate(nextProps, nextState) {
     return !shallowEqual(nextProps, this.props) || !shallowEqual(nextState, this.state)
   }
 
   componentDidUpdate(prevProps, prevState) {
     // eslint-disable-line complexity
+    super.componentDidUpdate(prevProps, prevState)
     debug('componentDidUpdate()')
     debug('to state:', objectDiff(prevState, this.state))
+    debug('changed props:', objectDiff(prevProps, this.props))
+
+    if (!shallowEqual(this.props.value, prevProps.value)) {
+      debug('value changed, setting', this.props.value)
+      this.setValue(this.props.value)
+    }
 
     // focused / blurred
     if (!prevState.focus && this.state.focus) {

diff --git a/test/specs/addons/Portal/Portal-test.js b/test/specs/addons/Portal/Portal-test.js
@@ -69,6 +69,7 @@ describe('Portal', () => {
 
       // Enzyme docs say it merges previous props but without children, react complains
       wrapper.setProps({ open: true, children: <p /> })
+      wrapper.update()
       wrapper.should.have.descendants(PortalInner)
     })
 
@@ -81,6 +82,7 @@ describe('Portal', () => {
       wrapper.should.have.descendants(PortalInner)
 
       wrapper.setProps({ open: false, children: <p /> })
+      wrapper.update()
       wrapper.should.not.have.descendants(PortalInner)
     })
   })