Update input pointers even when shouldComponentUpdate returns false

We should be able to reuse work even if it is pre-empted by a high
priority update before flushing. Because memoizedProps and memoizedState
are read from the instance, the instance's input pointers (props, state,
context) should be updated even when shouldComponentUpdate causes a
bail-out.

Author: acdlite

scripts/fiber/tests-failing.txt

@@ -1,6 +1,3 @@
-src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
-* does not do a deep comparison
-
 src/addons/__tests__/ReactFragment-test.js
 * should throw if a plain object is used as a child
 * should throw if a plain object even if it is in an owner

scripts/fiber/tests-passing.txt

@@ -36,6 +36,7 @@ scripts/error-codes/__tests__/invertObject-test.js
 
 src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js
 * provides a default shouldComponentUpdate implementation
+* does not do a deep comparison
 
 src/addons/__tests__/ReactFragment-test.js
 * warns for numeric keys on objects as children
@@ -1138,6 +1139,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
 * can resume work in a bailed subtree within one pass
 * can reuse work done after being preempted
 * can reuse work if shouldComponentUpdate is false, after being preempted
+* can reuse preempted work with shouldComponentUpdate
 * can update in the middle of a tree using setState
 * can queue multiple state updates
 * can use updater form of setState

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -251,9 +251,8 @@ module.exports = function<T, P, I, TI, C, CX>(
       // If an update was already in progress, we should schedule an Update
       // effect even though we're bailing out, so that cWU/cDU are called.
       if (current) {
-        const instance = current.stateNode;
-        if (instance.props !== current.memoizedProps ||
-            instance.state !== current.memoizedState) {
+        if (workInProgress.memoizedProps !== current.memoizedProps ||
+            workInProgress.memoizedState !== current.memoizedState) {
           workInProgress.effectTag |= Update;
         }
       }

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -303,6 +303,12 @@ module.exports = function(
       newState,
       newContext
     )) {
+      // Update the existing instance's state, props, and context pointers even
+      // though we're bailing out.
+      const instance = workInProgress.stateNode;
+      instance.props = newProps;
+      instance.state = newState;
+      instance.context = newContext;
       return false;
     }
 
@@ -385,25 +391,27 @@ module.exports = function(
       return false;
     }
 
-    if (!checkShouldComponentUpdate(
+    const shouldUpdate = checkShouldComponentUpdate(
       workInProgress,
       oldProps,
       newProps,
       newState,
       newContext
-    )) {
-      // TODO: Should this get the new props/state updated regardless?
-      return false;
-    }
+    );
 
-    if (typeof instance.componentWillUpdate === 'function') {
+    if (shouldUpdate && typeof instance.componentWillUpdate === 'function') {
       instance.componentWillUpdate(newProps, newState, newContext);
     }
 
+    // Update the existing instance's state, props, and context pointers even
+    // if shouldComponentUpdate returns false. memoizedProps and memoizedState
+    // are read from the instance, so by always updating them, we ensure that
+    // work is memoized even when we bail out.
     instance.props = newProps;
     instance.state = newState;
     instance.context = newContext;
-    return true;
+
+    return shouldUpdate;
   }
 
   return {

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

@@ -628,6 +628,55 @@ describe('ReactIncremental', () => {
 
   });
 
+  it('can reuse preempted work with shouldComponentUpdate', () => {
+    let ops = [];
+    class Foo extends React.Component {
+      shouldComponentUpdate(nextProps) {
+        const shouldUpdate = this.props.step !== nextProps.step;
+        ops.push('shouldComponentUpdate: ' + shouldUpdate);
+        return shouldUpdate;
+      }
+      render() {
+        ops.push('render');
+        return <div />;
+      }
+    }
+
+    ReactNoop.render(<Foo step={1} />);
+    ReactNoop.flush();
+    ops = [];
+
+    let didFlush = false;
+    ReactNoop.render(<Foo step={2} />, () => {
+      didFlush = true;
+    });
+    // Begin a low priority update, but stop before it is flushed.
+    ReactNoop.flushDeferredPri(15);
+    expect(ops).toEqual([
+      'shouldComponentUpdate: true',
+      'render',
+    ]);
+    expect(didFlush).toEqual(false);
+
+    ops = [];
+    // Preempt the low priority update with a higher one.
+    ReactNoop.performAnimationWork(() => {
+      ReactNoop.render(<Foo step={2} />);
+    });
+    ReactNoop.flush();
+
+    // shouldComponentUpdate should compare new props with preempted props,
+    // not current props. So it should return false and not re-render.
+    expect(ops).toEqual([
+      // There are two pending updates on the root, so sCU is called twice.
+      // High priority update
+      'shouldComponentUpdate: false',
+      // Low priority update
+      'shouldComponentUpdate: false',
+    ]);
+    expect(didFlush).toEqual(true);
+  });
+
   it('can update in the middle of a tree using setState', () => {
     let instance;
     class Bar extends React.Component {