diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 76af5ddf50746..6d7cb75bbd751 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; type Update = {| lane: Lane, action: A, - eagerReducer: ((S, A) => S) | null, + hasEagerState: boolean, eagerState: S | null, next: Update, |}; @@ -730,7 +730,7 @@ function mountReducer( lastRenderedState: (initialState: any), }; hook.queue = queue; - const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( + const dispatch: Dispatch = (queue.dispatch = (dispatchReducerAction.bind( null, currentlyRenderingFiber, queue, @@ -801,7 +801,7 @@ function updateReducer( const clone: Update = { lane: updateLane, action: update.action, - eagerReducer: update.eagerReducer, + hasEagerState: update.hasEagerState, eagerState: update.eagerState, next: (null: any), }; @@ -829,7 +829,7 @@ function updateReducer( // this will never be skipped by the check above. lane: NoLane, action: update.action, - eagerReducer: update.eagerReducer, + hasEagerState: update.hasEagerState, eagerState: update.eagerState, next: (null: any), }; @@ -837,9 +837,9 @@ function updateReducer( } // 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.hasEagerState) { + // If this update is a state update (not a reducer) and was processed eagerly, + // we can use the eagerly computed state newState = ((update.eagerState: any): S); } else { const action = update.action; @@ -1190,7 +1190,7 @@ function useMutableSource( lastRenderedReducer: basicStateReducer, lastRenderedState: snapshot, }; - newQueue.dispatch = setSnapshot = (dispatchAction.bind( + newQueue.dispatch = setSnapshot = (dispatchSetState.bind( null, currentlyRenderingFiber, newQueue, @@ -1481,7 +1481,7 @@ function mountState( hook.queue = queue; const dispatch: Dispatch< BasicStateAction, - > = (queue.dispatch = (dispatchAction.bind( + > = (queue.dispatch = (dispatchSetState.bind( null, currentlyRenderingFiber, queue, @@ -2150,7 +2150,7 @@ function refreshCache(fiber: Fiber, seedKey: ?() => T, seedValue: T) { // TODO: Warn if unmounted? } -function dispatchAction( +function dispatchReducerAction( fiber: Fiber, queue: UpdateQueue, action: A, @@ -2171,7 +2171,119 @@ function dispatchAction( const update: Update = { lane, action, - eagerReducer: null, + hasEagerState: false, + eagerState: null, + next: (null: any), + }; + + 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. + didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true; + 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; + } else { + if (isInterleavedUpdate(fiber, lane)) { + const interleaved = queue.interleaved; + if (interleaved === null) { + // This is the first update. Create a circular list. + update.next = update; + // At the end of the current render, this queue's interleaved updates will + // be transferred to the pending queue. + pushInterleavedQueue(queue); + } else { + update.next = interleaved.next; + interleaved.next = update; + } + queue.interleaved = update; + } else { + 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; + } + + if (__DEV__) { + // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests + if ('undefined' !== typeof jest) { + warnIfNotCurrentlyActingUpdatesInDev(fiber); + } + } + const root = scheduleUpdateOnFiber(fiber, lane, eventTime); + + if (isTransitionLane(lane) && root !== null) { + let queueLanes = queue.lanes; + + // If any entangled lanes are no longer pending on the root, then they + // must have finished. We can remove them from the shared queue, which + // represents a superset of the actually pending lanes. In some cases we + // may entangle more than we need to, but that's OK. In fact it's worse if + // we *don't* entangle when we should. + queueLanes = intersectLanes(queueLanes, root.pendingLanes); + + // Entangle the new transition lane with the other transition lanes. + const newQueueLanes = mergeLanes(queueLanes, lane); + queue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); + } + } + + if (__DEV__) { + if (enableDebugTracing) { + if (fiber.mode & DebugTracingMode) { + const name = getComponentNameFromFiber(fiber) || 'Unknown'; + logStateUpdateScheduled(name, lane, action); + } + } + } + + if (enableSchedulingProfiler) { + markStateUpdateScheduled(fiber, lane); + } +} + +function dispatchSetState( + fiber: Fiber, + queue: UpdateQueue, + action: A, +) { + if (__DEV__) { + if (typeof arguments[3] === 'function') { + console.error( + "State updates from the useState() and useReducer() Hooks don't support the " + + 'second callback argument. To execute a side effect after ' + + 'rendering, declare it in the component body with useEffect().', + ); + } + } + + const eventTime = requestEventTime(); + const lane = requestUpdateLane(fiber); + + const update: Update = { + lane, + action, + hasEagerState: false, eagerState: null, next: (null: any), }; @@ -2241,7 +2353,7 @@ function dispatchAction( // 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.hasEagerState = true; 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/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index b755852fe618d..75570d1c30969 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals; type Update = {| lane: Lane, action: A, - eagerReducer: ((S, A) => S) | null, + hasEagerState: boolean, eagerState: S | null, next: Update, |}; @@ -730,7 +730,7 @@ function mountReducer( lastRenderedState: (initialState: any), }; hook.queue = queue; - const dispatch: Dispatch = (queue.dispatch = (dispatchAction.bind( + const dispatch: Dispatch = (queue.dispatch = (dispatchReducerAction.bind( null, currentlyRenderingFiber, queue, @@ -801,7 +801,7 @@ function updateReducer( const clone: Update = { lane: updateLane, action: update.action, - eagerReducer: update.eagerReducer, + hasEagerState: update.hasEagerState, eagerState: update.eagerState, next: (null: any), }; @@ -829,7 +829,7 @@ function updateReducer( // this will never be skipped by the check above. lane: NoLane, action: update.action, - eagerReducer: update.eagerReducer, + hasEagerState: update.hasEagerState, eagerState: update.eagerState, next: (null: any), }; @@ -837,9 +837,9 @@ function updateReducer( } // 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.hasEagerState) { + // If this update is a state update (not a reducer) and was processed eagerly, + // we can use the eagerly computed state newState = ((update.eagerState: any): S); } else { const action = update.action; @@ -1190,7 +1190,7 @@ function useMutableSource( lastRenderedReducer: basicStateReducer, lastRenderedState: snapshot, }; - newQueue.dispatch = setSnapshot = (dispatchAction.bind( + newQueue.dispatch = setSnapshot = (dispatchSetState.bind( null, currentlyRenderingFiber, newQueue, @@ -1481,7 +1481,7 @@ function mountState( hook.queue = queue; const dispatch: Dispatch< BasicStateAction, - > = (queue.dispatch = (dispatchAction.bind( + > = (queue.dispatch = (dispatchSetState.bind( null, currentlyRenderingFiber, queue, @@ -2150,7 +2150,7 @@ function refreshCache(fiber: Fiber, seedKey: ?() => T, seedValue: T) { // TODO: Warn if unmounted? } -function dispatchAction( +function dispatchReducerAction( fiber: Fiber, queue: UpdateQueue, action: A, @@ -2171,7 +2171,119 @@ function dispatchAction( const update: Update = { lane, action, - eagerReducer: null, + hasEagerState: false, + eagerState: null, + next: (null: any), + }; + + 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. + didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true; + 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; + } else { + if (isInterleavedUpdate(fiber, lane)) { + const interleaved = queue.interleaved; + if (interleaved === null) { + // This is the first update. Create a circular list. + update.next = update; + // At the end of the current render, this queue's interleaved updates will + // be transferred to the pending queue. + pushInterleavedQueue(queue); + } else { + update.next = interleaved.next; + interleaved.next = update; + } + queue.interleaved = update; + } else { + 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; + } + + if (__DEV__) { + // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests + if ('undefined' !== typeof jest) { + warnIfNotCurrentlyActingUpdatesInDev(fiber); + } + } + const root = scheduleUpdateOnFiber(fiber, lane, eventTime); + + if (isTransitionLane(lane) && root !== null) { + let queueLanes = queue.lanes; + + // If any entangled lanes are no longer pending on the root, then they + // must have finished. We can remove them from the shared queue, which + // represents a superset of the actually pending lanes. In some cases we + // may entangle more than we need to, but that's OK. In fact it's worse if + // we *don't* entangle when we should. + queueLanes = intersectLanes(queueLanes, root.pendingLanes); + + // Entangle the new transition lane with the other transition lanes. + const newQueueLanes = mergeLanes(queueLanes, lane); + queue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); + } + } + + if (__DEV__) { + if (enableDebugTracing) { + if (fiber.mode & DebugTracingMode) { + const name = getComponentNameFromFiber(fiber) || 'Unknown'; + logStateUpdateScheduled(name, lane, action); + } + } + } + + if (enableSchedulingProfiler) { + markStateUpdateScheduled(fiber, lane); + } +} + +function dispatchSetState( + fiber: Fiber, + queue: UpdateQueue, + action: A, +) { + if (__DEV__) { + if (typeof arguments[3] === 'function') { + console.error( + "State updates from the useState() and useReducer() Hooks don't support the " + + 'second callback argument. To execute a side effect after ' + + 'rendering, declare it in the component body with useEffect().', + ); + } + } + + const eventTime = requestEventTime(); + const lane = requestUpdateLane(fiber); + + const update: Update = { + lane, + action, + hasEagerState: false, eagerState: null, next: (null: any), }; @@ -2241,7 +2353,7 @@ function dispatchAction( // 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.hasEagerState = true; 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.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index b687212f2b32c..f56577e8ffea9 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3867,7 +3867,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - it('eager bailout optimization should always compare to latest rendered reducer', () => { + it('useReducer does not eagerly bail out of state updates', () => { // Edge case based on a bug report let setCounter; function App() { @@ -3898,7 +3898,6 @@ describe('ReactHooksWithNoopRenderer', () => { 'Render: -1', 'Effect: 1', 'Reducer: 1', - 'Reducer: 1', 'Render: 1', ]); expect(ReactNoop).toMatchRenderedOutput('1'); @@ -3911,12 +3910,170 @@ describe('ReactHooksWithNoopRenderer', () => { 'Render: 1', 'Effect: 2', 'Reducer: 2', - 'Reducer: 2', 'Render: 2', ]); expect(ReactNoop).toMatchRenderedOutput('2'); }); + it('useReducer does not replay previous no-op actions when other state changes', () => { + let increment; + let setDisabled; + + function Counter() { + const [disabled, _setDisabled] = useState(true); + const [count, dispatch] = useReducer((state, action) => { + if (disabled) { + return state; + } + if (action.type === 'increment') { + return state + 1; + } + return state; + }, 0); + + increment = () => dispatch({type: 'increment'}); + setDisabled = _setDisabled; + + Scheduler.unstable_yieldValue('Render disabled: ' + disabled); + Scheduler.unstable_yieldValue('Render count: ' + count); + return count; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: true', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + // These increments should have no effect, since disabled=true + increment(); + increment(); + increment(); + }); + expect(Scheduler).toHaveYielded([ + 'Render disabled: true', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + // Enabling the updater should *not* replay the previous increment() actions + setDisabled(false); + }); + expect(Scheduler).toHaveYielded([ + 'Render disabled: false', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + }); + + it('useReducer does not replay previous no-op actions when props change', () => { + 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(() => { + // These increments should have no effect, since disabled=true + increment(); + increment(); + increment(); + }); + expect(Scheduler).toHaveYielded(['Render count: 0']); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + // Enabling the updater should *not* replay the previous increment() actions + setDisabled(false); + }); + expect(Scheduler).toHaveYielded([ + 'Render disabled: false', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + }); + + it('useReducer applies potential no-op changes if made relevant by other updates in the batch', () => { + 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(() => { + // Although the increment happens first (and would seem to do nothing since disabled=true), + // because these calls are in a batch the parent updates first. This should cause the child + // to re-render with disabled=false and *then* process the increment action, which now + // increments the count and causes the component output to change. + increment(); + setDisabled(false); + }); + expect(Scheduler).toHaveYielded([ + 'Render disabled: false', + 'Render count: 1', + ]); + expect(ReactNoop).toMatchRenderedOutput('1'); + }); + // Regression test. Covers a case where an internal state variable // (`didReceiveUpdate`) is not reset properly. it('state bail out edge case (#16359)', async () => {