From 474e99d0b788d683abe1099939cc29404c770a7f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 21 Nov 2019 14:20:47 -0800 Subject: [PATCH 1/2] Failing test: Updates "un-committed" when rebasing Adds a failing test case where an update that was committed is later skipped over during a rebase. This should never happen. --- .../ReactIncrementalUpdates-test.internal.js | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index 7ae9874105dc..095f3d8979a6 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -653,4 +653,68 @@ describe('ReactIncrementalUpdates', () => { expect(Scheduler).toFlushAndYield(['Commit: goodbye']); }); }); + + it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => { + const {useState, useLayoutEffect} = React; + + let pushToLog; + function App() { + const [log, setLog] = useState(''); + pushToLog = msg => { + setLog(prevLog => prevLog + msg); + }; + + useLayoutEffect( + () => { + Scheduler.unstable_yieldValue('Committed: ' + log); + if (log === 'B') { + // Right after B commits, schedule additional updates. + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => { + pushToLog('C'); + }, + ); + setLog(prevLog => prevLog + 'D'); + } + }, + [log], + ); + + return log; + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Committed: ']); + expect(root).toMatchRenderedOutput(''); + + await ReactNoop.act(async () => { + pushToLog('A'); + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => { + pushToLog('B'); + }, + ); + }); + expect(Scheduler).toHaveYielded([ + // A and B are pending. B is higher priority, so we'll render that first. + 'Committed: B', + // Because A comes first in the queue, we're now in rebase mode. B must + // be rebased on top of A. Also, in a layout effect, we received two new + // updates: C and D. C is user-blocking and D is synchronous. + // + // First render the synchronous update. What we're testing here is that + // B *is not dropped* even though it has lower than sync priority. That's + // because we already committed it. However, this render should not + // include C, because that update wasn't already committed. + 'Committed: BD', + 'Committed: BCD', + 'Committed: ABCD', + ]); + expect(root).toMatchRenderedOutput('ABCD'); + }); }); From 89ca3cf3019abee3d320bd15b5db6c893229ec94 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 27 Nov 2019 22:06:47 -0800 Subject: [PATCH 2/2] Refactor Hooks update queue --- .../react-reconciler/src/ReactFiberHooks.js | 119 ++++++++++-------- .../src/ReactFiberWorkLoop.js | 17 ++- 2 files changed, 78 insertions(+), 58 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 7a4c7dfae82b..0a6c3a132cc2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -20,7 +20,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import ReactSharedInternals from 'shared/ReactSharedInternals'; -import {NoWork} from './ReactFiberExpirationTime'; +import {NoWork, Sync} from './ReactFiberExpirationTime'; import {readContext} from './ReactFiberNewContext'; import {createResponderListener} from './ReactFiberEvents'; import { @@ -108,13 +108,13 @@ type Update = { action: A, eagerReducer: ((S, A) => S) | null, eagerState: S | null, - next: Update | null, + next: Update, priority?: ReactPriorityLevel, }; type UpdateQueue = { - last: Update | null, + pending: Update | null, dispatch: (A => mixed) | null, lastRenderedReducer: ((S, A) => S) | null, lastRenderedState: S | null, @@ -144,7 +144,7 @@ export type Hook = { memoizedState: any, baseState: any, - baseUpdate: Update | null, + baseQueue: Update | null, queue: UpdateQueue | null, next: Hook | null, @@ -544,8 +544,8 @@ function mountWorkInProgressHook(): Hook { memoizedState: null, baseState: null, + baseQueue: null, queue: null, - baseUpdate: null, next: null, }; @@ -604,8 +604,8 @@ function updateWorkInProgressHook(): Hook { memoizedState: currentHook.memoizedState, baseState: currentHook.baseState, + baseQueue: currentHook.baseQueue, queue: currentHook.queue, - baseUpdate: currentHook.baseUpdate, next: null, }; @@ -645,7 +645,7 @@ function mountReducer( } hook.memoizedState = hook.baseState = initialState; const queue = (hook.queue = { - last: null, + pending: null, dispatch: null, lastRenderedReducer: reducer, lastRenderedState: (initialState: any), @@ -703,7 +703,7 @@ function updateReducer( // the base state unless the queue is empty. // TODO: Not sure if this is the desired semantics, but it's what we // do for gDSFP. I can't remember why. - if (hook.baseUpdate === queue.last) { + if (hook.baseQueue === null) { hook.baseState = newState; } @@ -715,42 +715,55 @@ function updateReducer( return [hook.memoizedState, dispatch]; } - // The last update in the entire queue - const last = queue.last; - // The last update that is part of the base state. - const baseUpdate = hook.baseUpdate; - const baseState = hook.baseState; - - // Find the first unprocessed update. - let first; - if (baseUpdate !== null) { - if (last !== null) { - // For the first update, the queue is a circular linked list where - // `queue.last.next = queue.first`. Once the first update commits, and - // the `baseUpdate` is no longer empty, we can unravel the list. - last.next = null; + const current: Hook = (currentHook: any); + + // The last rebase update that is NOT part of the base state. + let baseQueue = current.baseQueue; + + // The last pending update that hasn't been processed yet. + let pendingQueue = queue.pending; + if (pendingQueue !== null) { + // We have new updates that haven't been processed yet. + // We'll add them to the base queue. + if (baseQueue !== null) { + // Merge the pending queue and the base queue. + let baseFirst = baseQueue.next; + let pendingFirst = pendingQueue.next; + baseQueue.next = pendingFirst; + pendingQueue.next = baseFirst; } - first = baseUpdate.next; - } else { - first = last !== null ? last.next : null; + current.baseQueue = baseQueue = pendingQueue; + queue.pending = null; } - if (first !== null) { - let newState = baseState; + + if (baseQueue !== null) { + // We have a queue to process. + let first = baseQueue.next; + let newState = current.baseState; + let newBaseState = null; - let newBaseUpdate = null; - let prevUpdate = baseUpdate; + let newBaseQueueFirst = null; + let newBaseQueueLast = null; let update = first; - let didSkip = false; do { const updateExpirationTime = update.expirationTime; if (updateExpirationTime < renderExpirationTime) { // Priority is insufficient. Skip this update. If this is the first // skipped update, the previous update/state is the new base // update/state. - if (!didSkip) { - didSkip = true; - newBaseUpdate = prevUpdate; + const clone: Update = { + expirationTime: update.expirationTime, + suspenseConfig: update.suspenseConfig, + action: update.action, + eagerReducer: update.eagerReducer, + eagerState: update.eagerState, + next: (null: any), + }; + if (newBaseQueueLast === null) { + newBaseQueueFirst = newBaseQueueLast = clone; newBaseState = newState; + } else { + newBaseQueueLast = newBaseQueueLast.next = clone; } // Update the remaining priority in the queue. if (updateExpirationTime > currentlyRenderingFiber.expirationTime) { @@ -760,6 +773,18 @@ function updateReducer( } else { // This update does have sufficient priority. + if (newBaseQueueLast !== null) { + const clone: Update = { + expirationTime: Sync, // This update is going to be committed so we never want uncommit it. + suspenseConfig: update.suspenseConfig, + action: update.action, + eagerReducer: update.eagerReducer, + eagerState: update.eagerState, + next: (null: any), + }; + newBaseQueueLast = newBaseQueueLast.next = clone; + } + // Mark the event time of this update as relevant to this render pass. // TODO: This should ideally use the true event time of this update rather than // its priority which is a derived and not reverseable value. @@ -781,13 +806,13 @@ function updateReducer( newState = reducer(newState, action); } } - prevUpdate = update; update = update.next; } while (update !== null && update !== first); - if (!didSkip) { - newBaseUpdate = prevUpdate; + if (newBaseQueueLast === null) { newBaseState = newState; + } else { + newBaseQueueLast.next = (newBaseQueueFirst: any); } // Mark that the fiber performed work, but only if the new state is @@ -797,8 +822,8 @@ function updateReducer( } hook.memoizedState = newState; - hook.baseUpdate = newBaseUpdate; hook.baseState = newBaseState; + hook.baseQueue = newBaseQueueLast; queue.lastRenderedState = newState; } @@ -816,7 +841,7 @@ function mountState( } hook.memoizedState = hook.baseState = initialState; const queue = (hook.queue = { - last: null, + pending: null, dispatch: null, lastRenderedReducer: basicStateReducer, lastRenderedState: (initialState: any), @@ -1233,7 +1258,7 @@ function dispatchAction( action, eagerReducer: null, eagerState: null, - next: null, + next: (null: any), }; if (__DEV__) { update.priority = getCurrentPriorityLevel(); @@ -1267,7 +1292,7 @@ function dispatchAction( action, eagerReducer: null, eagerState: null, - next: null, + next: (null: any), }; if (__DEV__) { @@ -1275,19 +1300,15 @@ function dispatchAction( } // Append the update to the end of the list. - const last = queue.last; - if (last === null) { + const pending = queue.pending; + if (pending === null) { // This is the first update. Create a circular list. update.next = update; } else { - const first = last.next; - if (first !== null) { - // Still circular. - update.next = first; - } - last.next = update; + update.next = pending.next; + pending.next = update; } - queue.last = update; + queue.pending = update; if ( fiber.expirationTime === NoWork && diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 8273d92b3c7a..8d0ced43911b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; +import type {Hook} from './ReactFiberHooks'; import { warnAboutDeprecatedLifecycles, @@ -2883,12 +2884,11 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { break; case FunctionComponent: case ForwardRef: - case SimpleMemoComponent: - if ( - workInProgressNode.memoizedState !== null && - workInProgressNode.memoizedState.baseUpdate !== null - ) { - let update = workInProgressNode.memoizedState.baseUpdate; + case SimpleMemoComponent: { + let firstHook: null | Hook = current.memoizedState; + // TODO: This just checks the first Hook. Isn't it suppose to check all Hooks? + if (firstHook !== null && firstHook.baseQueue !== null) { + let update = firstHook.baseQueue; // Loop through the functional component's memoized state to see whether // the component has triggered any high pri updates while (update !== null) { @@ -2908,15 +2908,14 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { } break; } - if ( - update.next === workInProgressNode.memoizedState.baseUpdate - ) { + if (update.next === firstHook.baseQueue) { break; } update = update.next; } } break; + } default: break; }