diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 851e22f8fe835..5cb32ca0fa46f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -103,7 +103,7 @@ type Update = {| expirationTime: ExpirationTime, suspenseConfig: null | SuspenseConfig, action: A, - eagerReducer: ((S, A) => S) | null, + eagerlyComputed: boolean, eagerState: S | null, next: Update, priority?: ReactPriorityLevel, @@ -112,7 +112,6 @@ type Update = {| type UpdateQueue = {| pending: Update | null, dispatch: (A => mixed) | null, - lastRenderedReducer: ((S, A) => S) | null, lastRenderedState: S | null, |}; @@ -641,7 +640,6 @@ function mountReducer( const queue = (hook.queue = { pending: null, dispatch: null, - lastRenderedReducer: reducer, lastRenderedState: (initialState: any), }); const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( @@ -664,8 +662,6 @@ function updateReducer( 'Should have a queue. This is likely a bug in React. Please file an issue.', ); - queue.lastRenderedReducer = reducer; - const current: Hook = (currentHook: any); // The last rebase update that is NOT part of the base state. @@ -706,7 +702,7 @@ function updateReducer( expirationTime: update.expirationTime, suspenseConfig: update.suspenseConfig, action: update.action, - eagerReducer: update.eagerReducer, + eagerlyComputed: update.eagerlyComputed, eagerState: update.eagerState, next: (null: any), }; @@ -729,7 +725,7 @@ function updateReducer( expirationTime: Sync, // This update is going to be committed so we never want uncommit it. suspenseConfig: update.suspenseConfig, action: update.action, - eagerReducer: update.eagerReducer, + eagerlyComputed: update.eagerlyComputed, eagerState: update.eagerState, next: (null: any), }; @@ -747,10 +743,9 @@ function updateReducer( update.suspenseConfig, ); - // Process this update. - if (update.eagerReducer === reducer) { - // If this update was processed eagerly, and its reducer matches the - // current reducer, we can use the eagerly computed state. + if (update.eagerlyComputed) { + // If this update was processed eagerly we can use the eagerly computed state. + // This can only happen for useState for which reducer is always the same. newState = ((update.eagerState: any): S); } else { const action = update.action; @@ -795,8 +790,6 @@ function rerenderReducer( 'Should have a queue. This is likely a bug in React. Please file an issue.', ); - queue.lastRenderedReducer = reducer; - // This is a re-render. Apply the new render phase updates to the previous // work-in-progress hook. const dispatch: Dispatch = (queue.dispatch: any); @@ -838,7 +831,7 @@ function rerenderReducer( } function mountState( - initialState: (() => S) | S, + initialState: BasicStateAction, ): [S, Dispatch>] { const hook = mountWorkInProgressHook(); if (typeof initialState === 'function') { @@ -849,12 +842,11 @@ function mountState( const queue = (hook.queue = { pending: null, dispatch: null, - lastRenderedReducer: basicStateReducer, lastRenderedState: (initialState: any), }); const dispatch: Dispatch< BasicStateAction, - > = (queue.dispatch = (dispatchAction.bind( + > = (queue.dispatch = (setState.bind( null, currentlyRenderingFiber, queue, @@ -863,7 +855,7 @@ function mountState( } function updateState( - initialState: (() => S) | S, + initialState: BasicStateAction, ): [S, Dispatch>] { return updateReducer(basicStateReducer, (initialState: any)); } @@ -1260,6 +1252,18 @@ function rerenderTransition( return [start, isPending]; } +function appendUpdate(queue: UpdateQueue, update: Update) { + const pending = queue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + update.next = pending.next; + pending.next = update; + } + queue.pending = update; +} + function dispatchAction( fiber: Fiber, queue: UpdateQueue, @@ -1268,7 +1272,7 @@ function dispatchAction( if (__DEV__) { if (typeof arguments[3] === 'function') { console.error( - "State updates from the useState() and useReducer() Hooks don't support the " + + "State updates from the useReducer() Hook don't support the " + 'second callback argument. To execute a side effect after ' + 'rendering, declare it in the component body with useEffect().', ); @@ -1287,7 +1291,7 @@ function dispatchAction( expirationTime, suspenseConfig, action, - eagerReducer: null, + eagerlyComputed: false, eagerState: null, next: (null: any), }; @@ -1296,22 +1300,73 @@ function dispatchAction( update.priority = getCurrentPriorityLevel(); } - // Append the update to the end of the list. - const pending = queue.pending; - if (pending === null) { - // This is the first update. Create a circular list. - update.next = update; + appendUpdate(queue, update); + + const alternate = fiber.alternate; + if ( + fiber === currentlyRenderingFiber || + (alternate !== null && alternate === currentlyRenderingFiber) + ) { + // This is a render phase update. Stash it in a lazily-created map of + // queue -> linked list of updates. After this render pass, we'll restart + // and apply the stashed updates on top of the work-in-progress hook. + didScheduleRenderPhaseUpdate = true; + update.expirationTime = renderExpirationTime; + currentlyRenderingFiber.expirationTime = renderExpirationTime; } else { - update.next = pending.next; - pending.next = update; + if (__DEV__) { + // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests + if ('undefined' !== typeof jest) { + warnIfNotScopedWithMatchingAct(fiber); + warnIfNotCurrentlyActingUpdatesInDev(fiber); + } + } + scheduleWork(fiber, expirationTime); + } +} + +function setState( + fiber: Fiber, + queue: UpdateQueue>, + action: BasicStateAction, +) { + if (__DEV__) { + if (typeof arguments[3] === 'function') { + console.error( + "State updates from the useState() Hook don't support the " + + 'second callback argument. To execute a side effect after ' + + 'rendering, declare it in the component body with useEffect().', + ); + } + } + + const currentTime = requestCurrentTimeForUpdate(); + const suspenseConfig = requestCurrentSuspenseConfig(); + const expirationTime = computeExpirationForFiber( + currentTime, + fiber, + suspenseConfig, + ); + + const update: Update> = { + expirationTime, + suspenseConfig, + action, + eagerlyComputed: false, + eagerState: null, + next: (null: any), + }; + + if (__DEV__) { + update.priority = getCurrentPriorityLevel(); } - queue.pending = update; const alternate = fiber.alternate; if ( fiber === currentlyRenderingFiber || (alternate !== null && alternate === currentlyRenderingFiber) ) { + appendUpdate(queue, update); // This is a render phase update. Stash it in a lazily-created map of // queue -> linked list of updates. After this render pass, we'll restart // and apply the stashed updates on top of the work-in-progress hook. @@ -1326,35 +1381,27 @@ 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 lastRenderedReducer = queue.lastRenderedReducer; - if (lastRenderedReducer !== null) { - let prevDispatcher; - if (__DEV__) { - prevDispatcher = ReactCurrentDispatcher.current; - ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV; + let prevDispatcher; + if (__DEV__) { + prevDispatcher = ReactCurrentDispatcher.current; + ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV; + } + try { + const currentState: S = (queue.lastRenderedState: any); + const eagerState = basicStateReducer(currentState, action); + if (is(eagerState, currentState)) { + // Fast path. We can bail out without scheduling React to re-render. + return; } - try { - 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 = lastRenderedReducer; - update.eagerState = eagerState; - if (is(eagerState, currentState)) { - // Fast path. We can bail out without scheduling React to re-render. - // It's still possible that we'll need to rebase this update later, - // if the component re-renders for a different reason and by that - // time the reducer has changed. - return; - } - } catch (error) { - // Suppress the error. It will throw again in the render phase. - } finally { - if (__DEV__) { - ReactCurrentDispatcher.current = prevDispatcher; - } + // Stash the eagerly computed state on the update object. + // It will be reused without without calling basicStateReducer again. + update.eagerlyComputed = true; + update.eagerState = eagerState; + } catch (error) { + // Suppress the error. It will throw again in the render phase. + } finally { + if (__DEV__) { + ReactCurrentDispatcher.current = prevDispatcher; } } } @@ -1365,6 +1412,8 @@ function dispatchAction( warnIfNotCurrentlyActingUpdatesInDev(fiber); } } + + appendUpdate(queue, update); scheduleWork(fiber, expirationTime); } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index f9073f1bf7018..d92c7023adb65 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -270,7 +270,7 @@ describe('ReactHooks', () => { }), ); }).toErrorDev( - 'State updates from the useState() and useReducer() Hooks ' + + 'State updates from the useState() Hook ' + "don't support the second callback argument. " + 'To execute a side effect after rendering, ' + 'declare it in the component body with useEffect().', @@ -304,7 +304,7 @@ describe('ReactHooks', () => { }), ); }).toErrorDev( - 'State updates from the useState() and useReducer() Hooks ' + + 'State updates from the useReducer() Hook ' + "don't support the second callback argument. " + 'To execute a side effect after rendering, ' + 'declare it in the component body with useEffect().', diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 81b774bd0a553..1567d03707a78 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -3030,56 +3030,6 @@ function loadModules({ }); }); - 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.unstable_yieldValue('Reducer: ' + count); - return count; - }, -1); - useEffect(() => { - Scheduler.unstable_yieldValue('Effect: ' + count); - dispatch(); - }, [count]); - Scheduler.unstable_yieldValue('Render: ' + state); - return count; - } - - act(() => { - ReactNoop.render(); - expect(Scheduler).toFlushAndYield([ - 'Render: -1', - 'Effect: 1', - 'Reducer: 1', - 'Reducer: 1', - 'Render: 1', - ]); - expect(ReactNoop).toMatchRenderedOutput('1'); - }); - - act(() => { - setCounter(2); - }); - expect(Scheduler).toHaveYielded([ - 'Render: 1', - 'Effect: 2', - 'Reducer: 2', - 'Reducer: 2', - 'Render: 2', - ]); - expect(ReactNoop).toMatchRenderedOutput('2'); - }); - // Regression test. Covers a case where an internal state variable // (`didReceiveUpdate`) is not reset properly. it('state bail out edge case (#16359)', async () => { @@ -3173,6 +3123,59 @@ function loadModules({ expect(ReactNoop).toMatchRenderedOutput('5'); }); + it('should not store dispatched items in the queue (TODO: better description)', () => { + let setDisabled; + let increment; + + function Counter({disabled}) { + const [count, dispatch] = useReducer((state, action) => { + if (disabled) { + return state; + } + if (action.type === 'increment') { + return state + 1; + } + return state; + }, 0); + + increment = () => dispatch({type: 'increment'}); + + Scheduler.unstable_yieldValue('Render count: ' + count); + return count; + } + + function App() { + const [disabled, _setDisabled] = useState(true); + setDisabled = _setDisabled; + Scheduler.unstable_yieldValue('Render disabled: ' + disabled); + return ; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: true', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + increment(); + increment(); + increment(); + }); + expect(Scheduler).toHaveYielded(['Render count: 0']); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + setDisabled(false); + }); + expect(Scheduler).toHaveYielded([ + 'Render disabled: false', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + }); + it('should process the rest pending updates after a render phase update', () => { // Similar to previous test, except using a preceding render phase update // instead of new props.