Don't recreate instance when resuming a class component's initial mount

scripts/fiber/tests-passing.txt

@@ -506,6 +506,7 @@ src/renderers/__tests__/ReactCompositeComponentState-test.js
 * should update state when called from child cWRP
 * should merge state when sCU returns false
 * should treat assigning to this.state inside cWRP as a replaceState, with a warning
+* should treat assigning to this.state inside cWM as a replaceState, with a warning
 
 src/renderers/__tests__/ReactEmptyComponent-test.js
 * should not produce child DOM nodes for null and false
@@ -1675,6 +1676,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
 * can resume work in a subtree even when a parent bails out
 * can resume work in a bailed subtree within one pass
 * can resume mounting a class component
+* reuses the same instance when resuming a class instance
 * can reuse work done after being preempted
 * can reuse work that began but did not complete, after being preempted
 * can reuse work if shouldComponentUpdate is false, after being preempted

src/renderers/__tests__/ReactCompositeComponentState-test.js

@@ -447,4 +447,44 @@ describe('ReactCompositeComponent-state', () => {
         'Use setState instead.',
     );
   });
+
+  it('should treat assigning to this.state inside cWM as a replaceState, with a warning', () => {
+    spyOn(console, 'error');
+
+    let ops = [];
+    class Test extends React.Component {
+      state = {step: 1, extra: true};
+      componentWillMount() {
+        this.setState({step: 2}, () => {
+          // Tests that earlier setState callbacks are not dropped
+          ops.push(
+            `callback -- step: ${this.state.step}, extra: ${!!this.state.extra}`,
+          );
+        });
+        // Treat like replaceState
+        this.state = {step: 3};
+      }
+      render() {
+        ops.push(
+          `render -- step: ${this.state.step}, extra: ${!!this.state.extra}`,
+        );
+        return null;
+      }
+    }
+
+    // Mount
+    const container = document.createElement('div');
+    ReactDOM.render(<Test />, container);
+
+    expect(ops).toEqual([
+      'render -- step: 3, extra: false',
+      'callback -- step: 3, extra: false',
+    ]);
+    expect(console.error.calls.count()).toEqual(1);
+    expect(console.error.calls.argsFor(0)[0]).toEqual(
+      'Warning: Test.componentWillMount(): Assigning directly to ' +
+        "this.state is deprecated (except inside a component's constructor). " +
+        'Use setState instead.',
+    );
+  });
 });

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -304,6 +304,59 @@ module.exports = function(
     return instance;
   }
 
+  function callComponentWillMount(workInProgress, instance) {
+    if (__DEV__) {
+      startPhaseTimer(workInProgress, 'componentWillMount');
+    }
+    const oldState = instance.state;
+    instance.componentWillMount();
+    if (__DEV__) {
+      stopPhaseTimer();
+    }
+
+    if (oldState !== instance.state) {
+      if (__DEV__) {
+        warning(
+          false,
+          '%s.componentWillMount(): Assigning directly to this.state is ' +
+            "deprecated (except inside a component's " +
+            'constructor). Use setState instead.',
+          getComponentName(workInProgress),
+        );
+      }
+      updater.enqueueReplaceState(instance, instance.state, null);
+    }
+  }
+
+  function callComponentWillReceiveProps(
+    workInProgress,
+    instance,
+    newProps,
+    newContext,
+  ) {
+    if (__DEV__) {
+      startPhaseTimer(workInProgress, 'componentWillReceiveProps');
+    }
+    const oldState = instance.state;
+    instance.componentWillReceiveProps(newProps, newContext);
+    if (__DEV__) {
+      stopPhaseTimer();
+    }
+
+    if (instance.state !== oldState) {
+      if (__DEV__) {
+        warning(
+          false,
+          '%s.componentWillReceiveProps(): Assigning directly to ' +
+            "this.state is deprecated (except inside a component's " +
+            'constructor). Use setState instead.',
+          getComponentName(workInProgress),
+        );
+      }
+      updater.enqueueReplaceState(instance, instance.state, null);
+    }
+  }
+
   // Invokes the mount life-cycles on a previously never rendered instance.
   function mountClassInstance(
     workInProgress: Fiber,
@@ -339,13 +392,7 @@ module.exports = function(
     }
 
     if (typeof instance.componentWillMount === 'function') {
-      if (__DEV__) {
-        startPhaseTimer(workInProgress, 'componentWillMount');
-      }
-      instance.componentWillMount();
-      if (__DEV__) {
-        stopPhaseTimer();
-      }
+      callComponentWillMount(workInProgress, instance);
       // If we had additional state updates during this life-cycle, let's
       // process them now.
       const updateQueue = workInProgress.updateQueue;
@@ -389,6 +436,34 @@ module.exports = function(
     const newUnmaskedContext = getUnmaskedContext(workInProgress);
     const newContext = getMaskedContext(workInProgress, newUnmaskedContext);
 
+    const oldContext = instance.context;
+    const oldProps = workInProgress.memoizedProps;
+
+    if (
+      typeof instance.componentWillReceiveProps === 'function' &&
+      (oldProps !== newProps || oldContext !== newContext)
+    ) {
+      callComponentWillReceiveProps(
+        workInProgress,
+        instance,
+        newProps,
+        newContext,
+      );
+    }
+
+    // Process the update queue before calling shouldComponentUpdate
+    const updateQueue = workInProgress.updateQueue;
+    if (updateQueue !== null) {
+      newState = beginUpdateQueue(
+        workInProgress,
+        updateQueue,
+        instance,
+        newState,
+        newProps,
+        priorityLevel,
+      );
+    }
+
     // TODO: Should we deal with a setState that happened after the last
     // componentWillMount and before this componentWillMount? Probably
     // unsupported anyway.
@@ -411,39 +486,34 @@ module.exports = function(
       return false;
     }
 
-    // If we didn't bail out we need to construct a new instance. We don't
-    // want to reuse one that failed to fully mount.
-    const newInstance = constructClassInstance(workInProgress, newProps);
-    newInstance.props = newProps;
-    newInstance.state = newState = newInstance.state || null;
-    newInstance.context = newContext;
+    // Update the input pointers now so that they are correct when we call
+    // componentWillMount
+    instance.props = newProps;
+    instance.state = newState;
+    instance.context = newContext;
 
-    if (typeof newInstance.componentWillMount === 'function') {
-      if (__DEV__) {
-        startPhaseTimer(workInProgress, 'componentWillMount');
-      }
-      newInstance.componentWillMount();
-      if (__DEV__) {
-        stopPhaseTimer();
+    if (typeof instance.componentWillMount === 'function') {
+      callComponentWillMount(workInProgress, instance);
+      // componentWillMount may have called setState. Process the update queue.
+      const newUpdateQueue = workInProgress.updateQueue;
+      if (newUpdateQueue !== null) {
+        newState = beginUpdateQueue(
+          workInProgress,
+          updateQueue,
+          instance,
+          newState,
+          newProps,
+          priorityLevel,
+        );
       }
     }
-    // If we had additional state updates, process them now.
-    // They may be from componentWillMount() or from error boundary's setState()
-    // during initial mounting.
-    const newUpdateQueue = workInProgress.updateQueue;
-    if (newUpdateQueue !== null) {
-      newInstance.state = beginUpdateQueue(
-        workInProgress,
-        newUpdateQueue,
-        newInstance,
-        newState,
-        newProps,
-        priorityLevel,
-      );
-    }
+
     if (typeof instance.componentDidMount === 'function') {
       workInProgress.effectTag |= Update;
     }
+
+    instance.state = newState;
+
     return true;
   }
 
@@ -476,29 +546,16 @@ module.exports = function(
     // ever the previously attempted to render - not the "current". However,
     // during componentDidUpdate we pass the "current" props.
 
-    if (oldProps !== newProps || oldContext !== newContext) {
-      if (typeof instance.componentWillReceiveProps === 'function') {
-        if (__DEV__) {
-          startPhaseTimer(workInProgress, 'componentWillReceiveProps');
-        }
-        instance.componentWillReceiveProps(newProps, newContext);
-        if (__DEV__) {
-          stopPhaseTimer();
-        }
-
-        if (instance.state !== workInProgress.memoizedState) {
-          if (__DEV__) {
-            warning(
-              false,
-              '%s.componentWillReceiveProps(): Assigning directly to ' +
-                "this.state is deprecated (except inside a component's " +
-                'constructor). Use setState instead.',
-              getComponentName(workInProgress),
-            );
-          }
-          updater.enqueueReplaceState(instance, instance.state, null);
-        }
-      }
+    if (
+      typeof instance.componentWillReceiveProps === 'function' &&
+      (oldProps !== newProps || oldContext !== newContext)
+    ) {
+      callComponentWillReceiveProps(
+        workInProgress,
+        instance,
+        newProps,
+        newContext,
+      );
     }
 
     // Compute the next state using the memoized state and the update queue.

src/renderers/shared/fiber/__tests__/ReactIncremental-test.js

@@ -490,7 +490,76 @@ describe('ReactIncremental', () => {
 
     ops = [];
     ReactNoop.flush();
-    expect(ops).toEqual(['Foo constructor: foo', 'Foo', 'Bar']);
+    expect(ops).toEqual(['Foo', 'Bar']);
+  });
+
+  it('reuses the same instance when resuming a class instance', () => {
+    let ops = [];
+    let foo;
+    class Parent extends React.Component {
+      shouldComponentUpdate() {
+        return false;
+      }
+      render() {
+        return <Foo prop={this.props.prop} />;
+      }
+    }
+
+    let constructorCount = 0;
+    class Foo extends React.Component {
+      constructor(props) {
+        super(props);
+        // Test based on a www bug where props was null on resume
+        ops.push('constructor: ' + props.prop);
+        constructorCount++;
+      }
+      componentWillMount() {
+        ops.push('componentWillMount: ' + this.props.prop);
+      }
+      componentWillReceiveProps() {
+        ops.push('componentWillReceiveProps: ' + this.props.prop);
+      }
+      componentDidMount() {
+        ops.push('componentDidMount: ' + this.props.prop);
+      }
+      componentWillUpdate() {
+        ops.push('componentWillUpdate: ' + this.props.prop);
+      }
+      componentDidUpdate() {
+        ops.push('componentDidUpdate: ' + this.props.prop);
+      }
+      render() {
+        foo = this;
+        ops.push('render: ' + this.props.prop);
+        return <Bar />;
+      }
+    }
+
+    function Bar() {
+      ops.push('Foo did complete');
+      return <div />;
+    }
+
+    ReactNoop.render(<Parent prop="foo" />);
+    ReactNoop.flushDeferredPri(25);
+    expect(ops).toEqual([
+      'constructor: foo',
+      'componentWillMount: foo',
+      'render: foo',
+      'Foo did complete',
+    ]);
+
+    foo.setState({value: 'bar'});
+
+    ops = [];
+    ReactNoop.flush();
+    expect(constructorCount).toEqual(1);
+    expect(ops).toEqual([
+      'componentWillMount: foo',
+      'render: foo',
+      'Foo did complete',
+      'componentDidMount: foo',
+    ]);
   });
 
   it('can reuse work done after being preempted', () => {
@@ -1030,7 +1099,7 @@ describe('ReactIncremental', () => {
     expect(ops).toEqual(['Foo', 'Bar:B2', 'Bar:D']);
 
     // We expect each rerender to correspond to a new instance.
-    expect(instances.size).toBe(5);
+    expect(instances.size).toBe(4);
   });
 
   it('gets new props when setting state on a partly updated component', () => {
@@ -1094,6 +1163,12 @@ describe('ReactIncremental', () => {
 
     class LifeCycle extends React.Component {
       state = {x: this.props.x};
+      componentWillReceiveProps(nextProps) {
+        ops.push(
+          'componentWillReceiveProps:' + this.state.x + '-' + nextProps.x,
+        );
+        this.setState({x: nextProps.x});
+      }
       componentWillMount() {
         ops.push('componentWillMount:' + this.state.x + '-' + this.props.x);
       }
@@ -1132,6 +1207,7 @@ describe('ReactIncremental', () => {
 
     expect(ops).toEqual([
       'App',
+      'componentWillReceiveProps:0-1',
       'componentWillMount:1-1',
       'Trail',
       'componentDidMount:1-1',
@@ -1179,7 +1255,7 @@ describe('ReactIncremental', () => {
 
     expect(ops).toEqual([
       'App',
-      'componentWillMount:1(ctor)',
+      'componentWillMount:0(willMount)',
       'render:1(willMount)',
       'componentDidMount:1(willMount)',
     ]);