Update debugRenderPhaseSideEffects behavior (facebook#12057)

Update debugRenderPhaseSideEffects behavior This feature flag no longer double-invokes componentWillMount, componentWillReceiveProps, componentWillUpdate, or shouldComponentUpdate. It continues to double-invoke the constructor, render, and setState updater functions as well as the recently added, static getDerivedStateFromProps method Tests have been updated.
bvaughn · Jan 24, 2018 · 431dca9 · 431dca9
1 parent d0e75dc
commit 431dca9
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 31 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -220,15 +220,6 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
         constructClassInstance(workInProgress, workInProgress.pendingProps);
         mountClassInstance(workInProgress, renderExpirationTime);
 
-        // Simulate an async bailout/interruption by invoking lifecycle twice.
-        // We do this here rather than inside of ReactFiberClassComponent,
-        // To more realistically simulate the interruption behavior of async,
-        // Which would never call componentWillMount() twice on the same instance.
-        if (debugRenderPhaseSideEffects) {
-          constructClassInstance(workInProgress, workInProgress.pendingProps);
-          mountClassInstance(workInProgress, renderExpirationTime);
-        }
-
         shouldUpdate = true;
       } else {
         invariant(false, 'Resuming work not yet implemented.');

diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js
@@ -199,11 +199,6 @@ export default function(
       );
       stopPhaseTimer();
 
-      // Simulate an async bailout/interruption by invoking lifecycle twice.
-      if (debugRenderPhaseSideEffects) {
-        instance.shouldComponentUpdate(newProps, newState, newContext);
-      }
-
       if (__DEV__) {
         warning(
           shouldUpdate !== undefined,
@@ -402,6 +397,12 @@ export default function(
     const context = needsContext
       ? getMaskedContext(workInProgress, unmaskedContext)
       : emptyObject;
+
+    // Instantiate twice to help detect side-effects.
+    if (debugRenderPhaseSideEffects) {
+      new ctor(props, context); // eslint-disable-line no-new
+    }
+
     const instance = new ctor(props, context);
     const state =
       instance.state !== null && instance.state !== undefined
@@ -537,11 +538,6 @@ export default function(
       startPhaseTimer(workInProgress, 'componentWillReceiveProps');
       instance.UNSAFE_componentWillReceiveProps(newProps, newContext);
       stopPhaseTimer();
-
-      // Simulate an async bailout/interruption by invoking lifecycle twice.
-      if (debugRenderPhaseSideEffects) {
-        instance.UNSAFE_componentWillReceiveProps(newProps, newContext);
-      }
     }
 
     if (instance.state !== oldState) {
@@ -589,6 +585,15 @@ export default function(
         }
       }
 
+      if (debugRenderPhaseSideEffects) {
+        // Invoke method an extra time to help detect side-effects.
+        type.getDerivedStateFromProps.call(
+          null,
+          props,
+          workInProgress.memoizedState,
+        );
+      }
+
       const partialState = type.getDerivedStateFromProps.call(
         null,
         props,
@@ -916,11 +921,6 @@ export default function(
           startPhaseTimer(workInProgress, 'componentWillUpdate');
           instance.UNSAFE_componentWillUpdate(newProps, newState, newContext);
           stopPhaseTimer();
-
-          // Simulate an async bailout/interruption by invoking lifecycle twice.
-          if (debugRenderPhaseSideEffects) {
-            instance.UNSAFE_componentWillUpdate(newProps, newState, newContext);
-          }
         }
       }
       if (typeof instance.componentDidUpdate === 'function') {

diff --git a/packages/react/src/__tests__/ReactAsyncClassComponent-test.internal.js b/packages/react/src/__tests__/ReactAsyncClassComponent-test.internal.js
@@ -28,6 +28,10 @@ describe('ReactAsyncClassComponent', () => {
       let shouldComponentUpdate = false;
       class ClassComponent extends React.Component {
         state = {};
+        static getDerivedStateFromProps() {
+          log.push('getDerivedStateFromProps');
+          return null;
+        }
         constructor(props) {
           super(props);
           log.push('constructor');
@@ -60,11 +64,21 @@ describe('ReactAsyncClassComponent', () => {
         }
       }
 
-      const component = ReactTestRenderer.create(<ClassComponent />);
+      let component;
+
+      expect(() => {
+        component = ReactTestRenderer.create(<ClassComponent />);
+      }).toWarnDev(
+        'ClassComponent: Defines both componentWillReceiveProps() ' +
+          'and static getDerivedStateFromProps() methods. ' +
+          'We recommend using only getDerivedStateFromProps().',
+      );
+
       expect(log).toEqual([
         'constructor',
-        'componentWillMount',
         'constructor',
+        'getDerivedStateFromProps',
+        'getDerivedStateFromProps',
         'componentWillMount',
         'render',
         'render',
@@ -77,11 +91,10 @@ describe('ReactAsyncClassComponent', () => {
       component.update(<ClassComponent />);
       expect(log).toEqual([
         'componentWillReceiveProps',
-        'componentWillReceiveProps',
-        'shouldComponentUpdate',
+        'getDerivedStateFromProps',
+        'getDerivedStateFromProps',
         'shouldComponentUpdate',
         'componentWillUpdate',
-        'componentWillUpdate',
         'render',
         'render',
         'componentDidUpdate',
@@ -93,8 +106,8 @@ describe('ReactAsyncClassComponent', () => {
       component.update(<ClassComponent />);
       expect(log).toEqual([
         'componentWillReceiveProps',
-        'componentWillReceiveProps',
-        'shouldComponentUpdate',
+        'getDerivedStateFromProps',
+        'getDerivedStateFromProps',
         'shouldComponentUpdate',
       ]);
     });