diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 03e453360250..655955072f40 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -89,8 +89,8 @@ type Update = { type UpdateQueue = { last: Update | null, dispatch: (A => mixed) | null, - eagerReducer: ((S, A) => S) | null, - eagerState: S | null, + lastRenderedReducer: ((S, A) => S) | null, + lastRenderedState: S | null, }; export type HookType = @@ -603,8 +603,8 @@ function mountReducer( const queue = (hook.queue = { last: null, dispatch: null, - eagerReducer: reducer, - eagerState: (initialState: any), + lastRenderedReducer: reducer, + lastRenderedState: (initialState: any), }); const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( null, @@ -627,6 +627,8 @@ function updateReducer( '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. @@ -662,8 +664,7 @@ function updateReducer( hook.baseState = newState; } - queue.eagerReducer = reducer; - queue.eagerState = newState; + queue.lastRenderedState = newState; return [newState, dispatch]; } @@ -742,8 +743,7 @@ function updateReducer( hook.baseUpdate = newBaseUpdate; hook.baseState = newBaseState; - queue.eagerReducer = reducer; - queue.eagerState = newState; + queue.lastRenderedState = newState; } const dispatch: Dispatch = (queue.dispatch: any); @@ -761,8 +761,8 @@ function mountState( const queue = (hook.queue = { last: null, dispatch: null, - eagerReducer: basicStateReducer, - eagerState: (initialState: any), + lastRenderedReducer: basicStateReducer, + lastRenderedState: (initialState: any), }); const dispatch: Dispatch< BasicStateAction, @@ -1141,21 +1141,21 @@ function dispatchAction( // 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. diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 647c38319bc0..8988181be26c 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1963,4 +1963,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 ; + } + + 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(); + 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(); + 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'); + }); });