diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 24a368056c0ad..61a14a94af4d1 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -1142,20 +1142,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { function logUpdateQueue(updateQueue: UpdateQueue, depth) { log(' '.repeat(depth + 1) + 'QUEUED UPDATES'); - const firstUpdate = updateQueue.firstUpdate; - if (!firstUpdate) { + const last = updateQueue.baseQueue; + if (last === null) { return; } + const first = last.next; + let update = first; + if (update !== null) { + do { + log( + ' '.repeat(depth + 1) + '~', + '[' + update.expirationTime + ']', + ); + } while (update !== null && update !== first); + } - log( - ' '.repeat(depth + 1) + '~', - '[' + firstUpdate.expirationTime + ']', - ); - while (firstUpdate.next) { - log( - ' '.repeat(depth + 1) + '~', - '[' + firstUpdate.expirationTime + ']', - ); + const lastPending = updateQueue.shared.pending; + if (lastPending !== null) { + const firstPending = lastPending.next; + let pendingUpdate = firstPending; + if (pendingUpdate !== null) { + do { + log( + ' '.repeat(depth + 1) + '~', + '[' + pendingUpdate.expirationTime + ']', + ); + } while (pendingUpdate !== null && pendingUpdate !== firstPending); + } } } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d916413a009e0..6610e0461a325 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1299,11 +1299,7 @@ function dispatchAction( // This is the first update. Create a circular list. update.next = update; } else { - const first = pending.next; - if (first !== null) { - // Still circular. - update.next = first; - } + update.next = pending.next; pending.next = update; } queue.pending = update; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 68531527da409..235978544cc4c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -2858,7 +2858,7 @@ export function checkForWrongSuspensePriorityInDEV(sourceFiber: Fiber) { // has triggered any high priority updates const updateQueue = current.updateQueue; if (updateQueue !== null) { - let update = updateQueue.firstUpdate; + let update = updateQueue.baseQueue; while (update !== null) { const priorityLevel = update.priority; if ( diff --git a/packages/react-reconciler/src/ReactUpdateQueue.js b/packages/react-reconciler/src/ReactUpdateQueue.js index 0d2a8f68230bf..10c7e55274e8c 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.js @@ -89,7 +89,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; -import {NoWork} from './ReactFiberExpirationTime'; +import {NoWork, Sync} from './ReactFiberExpirationTime'; import { enterDisallowedContextReadInDEV, exitDisallowedContextReadInDEV, @@ -117,27 +117,21 @@ export type Update = { payload: any, callback: (() => mixed) | null, - next: Update | null, - nextEffect: Update | null, + next: Update, //DEV only priority?: ReactPriorityLevel, }; +type SharedQueue = { + pending: Update | null, +}; + export type UpdateQueue = { baseState: State, - - firstUpdate: Update | null, - lastUpdate: Update | null, - - firstCapturedUpdate: Update | null, - lastCapturedUpdate: Update | null, - - firstEffect: Update | null, - lastEffect: Update | null, - - firstCapturedEffect: Update | null, - lastCapturedEffect: Update | null, + baseQueue: Update | null, + shared: SharedQueue, + effects: Array> | null, }; export const UpdateState = 0; @@ -161,17 +155,14 @@ if (__DEV__) { }; } -export function createUpdateQueue(baseState: State): UpdateQueue { +function createUpdateQueue(fiber: Fiber): UpdateQueue { const queue: UpdateQueue = { - baseState, - firstUpdate: null, - lastUpdate: null, - firstCapturedUpdate: null, - lastCapturedUpdate: null, - firstEffect: null, - lastEffect: null, - firstCapturedEffect: null, - lastCapturedEffect: null, + baseState: fiber.memoizedState, + baseQueue: null, + shared: { + pending: null, + }, + effects: null, }; return queue; } @@ -181,19 +172,9 @@ function cloneUpdateQueue( ): UpdateQueue { const queue: UpdateQueue = { baseState: currentQueue.baseState, - firstUpdate: currentQueue.firstUpdate, - lastUpdate: currentQueue.lastUpdate, - - // TODO: With resuming, if we bail out and resuse the child tree, we should - // keep these effects. - firstCapturedUpdate: null, - lastCapturedUpdate: null, - - firstEffect: null, - lastEffect: null, - - firstCapturedEffect: null, - lastCapturedEffect: null, + baseQueue: currentQueue.baseQueue, + shared: currentQueue.shared, + effects: null, }; return queue; } @@ -210,90 +191,46 @@ export function createUpdate( payload: null, callback: null, - next: null, - nextEffect: null, + next: (null: any), }; + update.next = update; if (__DEV__) { update.priority = getCurrentPriorityLevel(); } return update; } -function appendUpdateToQueue( - queue: UpdateQueue, - update: Update, -) { - // Append the update to the end of the list. - if (queue.lastUpdate === null) { - // Queue is empty - queue.firstUpdate = queue.lastUpdate = update; - } else { - queue.lastUpdate.next = update; - queue.lastUpdate = update; - } -} - export function enqueueUpdate(fiber: Fiber, update: Update) { - // Update queues are created lazily. - const alternate = fiber.alternate; - let queue1; - let queue2; - if (alternate === null) { - // There's only one fiber. - queue1 = fiber.updateQueue; - queue2 = null; - if (queue1 === null) { - queue1 = fiber.updateQueue = createUpdateQueue(fiber.memoizedState); - } - } else { - // There are two owners. - queue1 = fiber.updateQueue; - queue2 = alternate.updateQueue; - if (queue1 === null) { - if (queue2 === null) { - // Neither fiber has an update queue. Create new ones. - queue1 = fiber.updateQueue = createUpdateQueue(fiber.memoizedState); - queue2 = alternate.updateQueue = createUpdateQueue( - alternate.memoizedState, - ); - } else { - // Only one fiber has an update queue. Clone to create a new one. - queue1 = fiber.updateQueue = cloneUpdateQueue(queue2); - } + let sharedQueue; + let updateQueue = fiber.updateQueue; + if (updateQueue === null) { + const alternate = fiber.alternate; + if (alternate === null) { + updateQueue = createUpdateQueue(fiber); } else { - if (queue2 === null) { - // Only one fiber has an update queue. Clone to create a new one. - queue2 = alternate.updateQueue = cloneUpdateQueue(queue1); - } else { - // Both owners have an update queue. + updateQueue = alternate.updateQueue; + if (updateQueue === null) { + updateQueue = alternate.updateQueue = createUpdateQueue(alternate); } } + fiber.updateQueue = updateQueue; } - if (queue2 === null || queue1 === queue2) { - // There's only a single queue. - appendUpdateToQueue(queue1, update); + sharedQueue = updateQueue.shared; + + const pending = sharedQueue.pending; + if (pending === null) { + // This is the first update. Create a circular list. + update.next = update; } else { - // There are two queues. We need to append the update to both queues, - // while accounting for the persistent structure of the list — we don't - // want the same update to be added multiple times. - if (queue1.lastUpdate === null || queue2.lastUpdate === null) { - // One of the queues is not empty. We must add the update to both queues. - appendUpdateToQueue(queue1, update); - appendUpdateToQueue(queue2, update); - } else { - // Both queues are non-empty. The last update is the same in both lists, - // because of structural sharing. So, only append to one of the lists. - appendUpdateToQueue(queue1, update); - // But we still need to update the `lastUpdate` pointer of queue2. - queue2.lastUpdate = update; - } + update.next = pending.next; + pending.next = update; } + sharedQueue.pending = update; if (__DEV__) { if ( fiber.tag === ClassComponent && - (currentlyProcessingQueue === queue1 || - (queue2 !== null && currentlyProcessingQueue === queue2)) && + currentlyProcessingQueue === sharedQueue && !didWarnUpdateInsideUpdate ) { warningWithoutStack( @@ -312,12 +249,11 @@ export function enqueueCapturedUpdate( workInProgress: Fiber, update: Update, ) { - // Captured updates go into a separate list, and only on the work-in- - // progress queue. + // Captured updates go only on the work-in-progress queue. let workInProgressQueue = workInProgress.updateQueue; if (workInProgressQueue === null) { workInProgressQueue = workInProgress.updateQueue = createUpdateQueue( - workInProgress.memoizedState, + workInProgress, ); } else { // TODO: I put this here rather than createWorkInProgress so that we don't @@ -330,12 +266,13 @@ export function enqueueCapturedUpdate( } // Append the update to the end of the list. - if (workInProgressQueue.lastCapturedUpdate === null) { - // This is the first render phase update - workInProgressQueue.firstCapturedUpdate = workInProgressQueue.lastCapturedUpdate = update; + const last = workInProgressQueue.baseQueue; + if (last === null) { + workInProgressQueue.baseQueue = update.next = update; + update.next = update; } else { - workInProgressQueue.lastCapturedUpdate.next = update; - workInProgressQueue.lastCapturedUpdate = update; + update.next = last.next; + last.next = update; } } @@ -439,148 +376,162 @@ export function processUpdateQueue( queue = ensureWorkInProgressQueueIsAClone(workInProgress, queue); if (__DEV__) { - currentlyProcessingQueue = queue; + currentlyProcessingQueue = queue.shared; } - // These values may change as we process the queue. - let newBaseState = queue.baseState; - let newFirstUpdate = null; - let newExpirationTime = NoWork; - - // Iterate through the list of updates to compute the result. - let update = queue.firstUpdate; - let resultState = newBaseState; - while (update !== null) { - const updateExpirationTime = update.expirationTime; - if (updateExpirationTime < renderExpirationTime) { - // This update does not have sufficient priority. Skip it. - if (newFirstUpdate === null) { - // This is the first skipped update. It will be the first update in - // the new list. - newFirstUpdate = update; - // Since this is the first update that was skipped, the current result - // is the new base state. - newBaseState = resultState; - } - // Since this update will remain in the list, update the remaining - // expiration time. - if (newExpirationTime < updateExpirationTime) { - newExpirationTime = updateExpirationTime; - } - } else { - // This update does have sufficient priority. - - // 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. - // TODO: We should skip this update if it was already committed but currently - // we have no way of detecting the difference between a committed and suspended - // update here. - markRenderEventTimeAndConfig(updateExpirationTime, update.suspenseConfig); - - // Process it and compute a new result. - resultState = getStateFromUpdate( - workInProgress, - queue, - update, - resultState, - props, - instance, - ); - const callback = update.callback; - if (callback !== null) { - workInProgress.effectTag |= Callback; - // Set this to null, in case it was mutated during an aborted render. - update.nextEffect = null; - if (queue.lastEffect === null) { - queue.firstEffect = queue.lastEffect = update; - } else { - queue.lastEffect.nextEffect = update; - queue.lastEffect = update; - } + // The last rebase update that is NOT part of the base state. + let baseQueue = queue.baseQueue; + + // The last pending update that hasn't been processed yet. + let pendingQueue = queue.shared.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; + } + + baseQueue = pendingQueue; + + queue.shared.pending = null; + // TODO: Pass `current` as argument + const current = workInProgress.alternate; + if (current !== null) { + const currentQueue = current.updateQueue; + if (currentQueue !== null) { + currentQueue.baseQueue = pendingQueue; } } - // Continue to the next update. - update = update.next; } - // Separately, iterate though the list of captured updates. - let newFirstCapturedUpdate = null; - update = queue.firstCapturedUpdate; - while (update !== null) { - const updateExpirationTime = update.expirationTime; - if (updateExpirationTime < renderExpirationTime) { - // This update does not have sufficient priority. Skip it. - if (newFirstCapturedUpdate === null) { - // This is the first skipped captured update. It will be the first - // update in the new list. - newFirstCapturedUpdate = update; - // If this is the first update that was skipped, the current result is - // the new base state. - if (newFirstUpdate === null) { - newBaseState = resultState; - } - } - // Since this update will remain in the list, update the remaining - // expiration time. - if (newExpirationTime < updateExpirationTime) { - newExpirationTime = updateExpirationTime; - } - } else { - // This update does have sufficient priority. Process it and compute - // a new result. - resultState = getStateFromUpdate( - workInProgress, - queue, - update, - resultState, - props, - instance, - ); - const callback = update.callback; - if (callback !== null) { - workInProgress.effectTag |= Callback; - // Set this to null, in case it was mutated during an aborted render. - update.nextEffect = null; - if (queue.lastCapturedEffect === null) { - queue.firstCapturedEffect = queue.lastCapturedEffect = update; + // These values may change as we process the queue. + if (baseQueue !== null) { + let first = baseQueue.next; + // Iterate through the list of updates to compute the result. + let newState = queue.baseState; + let newExpirationTime = NoWork; + + let newBaseState = null; + let newBaseQueueFirst = null; + let newBaseQueueLast = null; + + if (first !== null) { + let update = first; + 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. + const clone: Update = { + expirationTime: update.expirationTime, + suspenseConfig: update.suspenseConfig, + + tag: update.tag, + payload: update.payload, + callback: update.callback, + + 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 > newExpirationTime) { + newExpirationTime = updateExpirationTime; + } } else { - queue.lastCapturedEffect.nextEffect = update; - queue.lastCapturedEffect = update; + // 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, + + tag: update.tag, + payload: update.payload, + callback: update.callback, + + 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. + // TODO: We should skip this update if it was already committed but currently + // we have no way of detecting the difference between a committed and suspended + // update here. + markRenderEventTimeAndConfig( + updateExpirationTime, + update.suspenseConfig, + ); + + // Process this update. + newState = getStateFromUpdate( + workInProgress, + queue, + update, + newState, + props, + instance, + ); + const callback = update.callback; + if (callback !== null) { + workInProgress.effectTag |= Callback; + let effects = queue.effects; + if (effects === null) { + queue.effects = [update]; + } else { + effects.push(update); + } + } } - } + update = update.next; + if (update === null || update === first) { + pendingQueue = queue.shared.pending; + if (pendingQueue === null) { + break; + } else { + // An update was scheduled from inside a reducer. Add the new + // pending updates to the end of the list and keep processing. + update = baseQueue.next = pendingQueue.next; + pendingQueue.next = first; + queue.baseQueue = baseQueue = pendingQueue; + queue.shared.pending = null; + } + } + } while (true); } - update = update.next; - } - if (newFirstUpdate === null) { - queue.lastUpdate = null; - } - if (newFirstCapturedUpdate === null) { - queue.lastCapturedUpdate = null; - } else { - workInProgress.effectTag |= Callback; - } - if (newFirstUpdate === null && newFirstCapturedUpdate === null) { - // We processed every update, without skipping. That means the new base - // state is the same as the result state. - newBaseState = resultState; - } + if (newBaseQueueLast === null) { + newBaseState = newState; + } else { + newBaseQueueLast.next = (newBaseQueueFirst: any); + } - queue.baseState = newBaseState; - queue.firstUpdate = newFirstUpdate; - queue.firstCapturedUpdate = newFirstCapturedUpdate; - - // Set the remaining expiration time to be whatever is remaining in the queue. - // This should be fine because the only two other things that contribute to - // expiration time are props and context. We're already in the middle of the - // begin phase by the time we start processing the queue, so we've already - // dealt with the props. Context in components that specify - // shouldComponentUpdate is tricky; but we'll have to account for - // that regardless. - markUnprocessedUpdateTime(newExpirationTime); - workInProgress.expirationTime = newExpirationTime; - workInProgress.memoizedState = resultState; + queue.baseState = ((newBaseState: any): State); + queue.baseQueue = newBaseQueueLast; + + // Set the remaining expiration time to be whatever is remaining in the queue. + // This should be fine because the only two other things that contribute to + // expiration time are props and context. We're already in the middle of the + // begin phase by the time we start processing the queue, so we've already + // dealt with the props. Context in components that specify + // shouldComponentUpdate is tricky; but we'll have to account for + // that regardless. + markUnprocessedUpdateTime(newExpirationTime); + workInProgress.expirationTime = newExpirationTime; + workInProgress.memoizedState = newState; + } if (__DEV__) { currentlyProcessingQueue = null; @@ -611,38 +562,17 @@ export function commitUpdateQueue( instance: any, renderExpirationTime: ExpirationTime, ): void { - // If the finished render included captured updates, and there are still - // lower priority updates left over, we need to keep the captured updates - // in the queue so that they are rebased and not dropped once we process the - // queue again at the lower priority. - if (finishedQueue.firstCapturedUpdate !== null) { - // Join the captured update list to the end of the normal list. - if (finishedQueue.lastUpdate !== null) { - finishedQueue.lastUpdate.next = finishedQueue.firstCapturedUpdate; - finishedQueue.lastUpdate = finishedQueue.lastCapturedUpdate; - } - // Clear the list of captured updates. - finishedQueue.firstCapturedUpdate = finishedQueue.lastCapturedUpdate = null; - } - // Commit the effects - commitUpdateEffects(finishedQueue.firstEffect, instance); - finishedQueue.firstEffect = finishedQueue.lastEffect = null; - - commitUpdateEffects(finishedQueue.firstCapturedEffect, instance); - finishedQueue.firstCapturedEffect = finishedQueue.lastCapturedEffect = null; -} - -function commitUpdateEffects( - effect: Update | null, - instance: any, -): void { - while (effect !== null) { - const callback = effect.callback; - if (callback !== null) { - effect.callback = null; - callCallback(callback, instance); + const effects = finishedQueue.effects; + finishedQueue.effects = null; + if (effects !== null) { + for (let i = 0; i < effects.length; i++) { + const effect = effects[i]; + const callback = effect.callback; + if (callback !== null) { + effect.callback = null; + callCallback(callback, instance); + } } - effect = effect.nextEffect; } } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index 832e1a55f4259..7d199e2522d9d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -718,6 +718,66 @@ describe('ReactIncrementalUpdates', () => { expect(root).toMatchRenderedOutput('ABCD'); }); + it('when rebasing, does not exclude updates that were already committed, regardless of priority (classes)', async () => { + let pushToLog; + class App extends React.Component { + state = {log: ''}; + pushToLog = msg => { + this.setState(prevState => ({log: prevState.log + msg})); + }; + componentDidUpdate() { + Scheduler.unstable_yieldValue('Committed: ' + this.state.log); + if (this.state.log === 'B') { + // Right after B commits, schedule additional updates. + Scheduler.unstable_runWithPriority( + Scheduler.unstable_UserBlockingPriority, + () => { + this.pushToLog('C'); + }, + ); + this.pushToLog('D'); + } + } + render() { + pushToLog = this.pushToLog; + return this.state.log; + } + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([]); + 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'); + }); + it("base state of update queue is initialized to its fiber's memoized state", async () => { // This test is very weird because it tests an implementation detail but // is tested in terms of public APIs. When it was originally written, the