Eager bailout optimization should always compare to latest reducer (facebook#15124)

* Eager bailout optimization should always compare to latest reducer

* queue.eagerReducer -> queue.lastRenderedReducer

This name is a bit more descriptive.

* Add test case that uses preceding render phase update

Author: acdlite

packages/react-reconciler/src/ReactFiberHooks.js

@@ -89,8 +89,8 @@ type Update<S, A> = {
 type UpdateQueue<S, A> = {
   last: Update<S, A> | null,
   dispatch: (A => mixed) | null,
-  eagerReducer: ((S, A) => S) | null,
-  eagerState: S | null,
+  lastRenderedReducer: ((S, A) => S) | null,
+  lastRenderedState: S | null,
 };
 
 export type HookType =
@@ -591,8 +591,8 @@ function mountReducer<S, I, A>(
   const queue = (hook.queue = {
     last: null,
     dispatch: null,
-    eagerReducer: reducer,
-    eagerState: (initialState: any),
+    lastRenderedReducer: reducer,
+    lastRenderedState: (initialState: any),
   });
   const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
     null,
@@ -615,6 +615,8 @@ function updateReducer<S, I, A>(
     'Should have a queue. This is likely a bug in React. Please file an issue.',
   );
 
+  queue.lastRenderedReducer = reducer;
+
   if (numberOfReRenders > 0) {
     // This is a re-render. Apply the new render phase updates to the previous
     // work-in-progress hook.
@@ -650,8 +652,7 @@ function updateReducer<S, I, A>(
           hook.baseState = newState;
         }
 
-        queue.eagerReducer = reducer;
-        queue.eagerState = newState;
+        queue.lastRenderedState = newState;
 
         return [newState, dispatch];
       }
@@ -730,8 +731,7 @@ function updateReducer<S, I, A>(
     hook.baseUpdate = newBaseUpdate;
     hook.baseState = newBaseState;
 
-    queue.eagerReducer = reducer;
-    queue.eagerState = newState;
+    queue.lastRenderedState = newState;
   }
 
   const dispatch: Dispatch<A> = (queue.dispatch: any);
@@ -749,8 +749,8 @@ function mountState<S>(
   const queue = (hook.queue = {
     last: null,
     dispatch: null,
-    eagerReducer: basicStateReducer,
-    eagerState: (initialState: any),
+    lastRenderedReducer: basicStateReducer,
+    lastRenderedState: (initialState: any),
   });
   const dispatch: Dispatch<
     BasicStateAction<S>,
@@ -1129,21 +1129,21 @@ function dispatchAction<S, A>(
       // The queue is currently empty, which means we can eagerly compute the
       // next state before entering the render phase. If the new state is the
       // same as the current state, we may be able to bail out entirely.
-      const eagerReducer = queue.eagerReducer;
-      if (eagerReducer !== null) {
+      const lastRenderedReducer = queue.lastRenderedReducer;
+      if (lastRenderedReducer !== null) {
         let prevDispatcher;
         if (__DEV__) {
           prevDispatcher = ReactCurrentDispatcher.current;
           ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
         }
         try {
-          const currentState: S = (queue.eagerState: any);
-          const eagerState = eagerReducer(currentState, action);
+          const currentState: S = (queue.lastRenderedState: any);
+          const eagerState = lastRenderedReducer(currentState, action);
           // Stash the eagerly computed state, and the reducer used to compute
           // it, on the update object. If the reducer hasn't changed by the
           // time we enter the render phase, then the eager state can be used
           // without calling the reducer again.
-          update.eagerReducer = eagerReducer;
+          update.eagerReducer = lastRenderedReducer;
           update.eagerState = eagerState;
           if (is(eagerState, currentState)) {
             // Fast path. We can bail out without scheduling React to re-render.

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

@@ -1871,4 +1871,88 @@ describe('ReactHooksWithNoopRenderer', () => {
       // );
     });
   });
+
+  it('eager bailout optimization should always compare to latest rendered reducer', () => {
+    // Edge case based on a bug report
+    let setCounter;
+    function App() {
+      const [counter, _setCounter] = useState(1);
+      setCounter = _setCounter;
+      return <Component count={counter} />;
+    }
+
+    function Component({count}) {
+      const [state, dispatch] = useReducer(() => {
+        // This reducer closes over a value from props. If the reducer is not
+        // properly updated, the eager reducer will compare to an old value
+        // and bail out incorrectly.
+        Scheduler.yieldValue('Reducer: ' + count);
+        return count;
+      }, -1);
+      useEffect(
+        () => {
+          Scheduler.yieldValue('Effect: ' + count);
+          dispatch();
+        },
+        [count],
+      );
+      Scheduler.yieldValue('Render: ' + state);
+      return count;
+    }
+
+    ReactNoop.render(<App />);
+    expect(Scheduler).toFlushAndYield([
+      'Render: -1',
+      'Effect: 1',
+      'Reducer: 1',
+      'Reducer: 1',
+      'Render: 1',
+    ]);
+    expect(ReactNoop).toMatchRenderedOutput('1');
+
+    act(() => {
+      setCounter(2);
+    });
+    expect(Scheduler).toFlushAndYield([
+      'Render: 1',
+      'Effect: 2',
+      'Reducer: 2',
+      'Reducer: 2',
+      'Render: 2',
+    ]);
+    expect(ReactNoop).toMatchRenderedOutput('2');
+  });
+
+  it('should update latest rendered reducer when a preceding state receives a render phase update', () => {
+    // Similar to previous test, except using a preceding render phase update
+    // instead of new props.
+    let dispatch;
+    function App() {
+      const [step, setStep] = useState(0);
+      const [shadow, _dispatch] = useReducer(() => step, step);
+      dispatch = _dispatch;
+
+      if (step < 5) {
+        setStep(step + 1);
+      }
+
+      Scheduler.yieldValue(`Step: ${step}, Shadow: ${shadow}`);
+      return shadow;
+    }
+
+    ReactNoop.render(<App />);
+    expect(Scheduler).toFlushAndYield([
+      'Step: 0, Shadow: 0',
+      'Step: 1, Shadow: 0',
+      'Step: 2, Shadow: 0',
+      'Step: 3, Shadow: 0',
+      'Step: 4, Shadow: 0',
+      'Step: 5, Shadow: 0',
+    ]);
+    expect(ReactNoop).toMatchRenderedOutput('0');
+
+    act(() => dispatch());
+    expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']);
+    expect(ReactNoop).toMatchRenderedOutput('5');
+  });
 });