From 7bf40e1cfdb780788700a41bf30163fdb8d105a3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 10 Dec 2019 18:42:42 -0600 Subject: [PATCH] Initialize update queue object on mount (#17560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Refactor Update Queues to Fix Rebasing Bug Fixes a bug related to rebasing updates. Once an update has committed, it should never un-commit, even if interrupted by a higher priority update. The fix includes a refactor of how update queues work. This commit is a combination of two PRs: - #17483 by @sebmarkbage refactors the hook update queue - #17510 by @acdlite refactors the class and root update queue Landing one without the other would cause state updates to sometimes be inconsistent across components, so I've combined them into a single commit in case they need to be reverted. Co-authored-by: Sebastian Markbåge Co-authored-by: Andrew Clark * Initialize update queue object on mount Instead of lazily initializing update queue objects on the first update, class and host root queues are created on mount. This simplifies the logic for appending new updates and matches what we do for hooks. --- .../src/createReactNoop.js | 35 +- .../src/ReactFiberBeginWork.js | 19 +- .../src/ReactFiberClassComponent.js | 69 +-- .../react-reconciler/src/ReactFiberHooks.js | 119 ++-- .../react-reconciler/src/ReactFiberRoot.js | 3 + .../src/ReactFiberWorkLoop.js | 19 +- .../react-reconciler/src/ReactUpdateQueue.js | 536 +++++++----------- .../ReactIncrementalUpdates-test.internal.js | 172 ++++++ 8 files changed, 530 insertions(+), 442 deletions(-) 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/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 678e1398d39c1..3d3342e9ae49b 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -90,7 +90,11 @@ import { reconcileChildFibers, cloneChildFibers, } from './ReactChildFiber'; -import {processUpdateQueue} from './ReactUpdateQueue'; +import { + processUpdateQueue, + cloneUpdateQueue, + initializeUpdateQueue, +} from './ReactUpdateQueue'; import { NoWork, Never, @@ -904,7 +908,7 @@ function updateHostRoot(current, workInProgress, renderExpirationTime) { pushHostRootContext(workInProgress); const updateQueue = workInProgress.updateQueue; invariant( - updateQueue !== null, + current !== null && updateQueue !== null, 'If the root does not have an updateQueue, we should have already ' + 'bailed out. This error is likely caused by a bug in React. Please ' + 'file an issue.', @@ -912,13 +916,8 @@ function updateHostRoot(current, workInProgress, renderExpirationTime) { const nextProps = workInProgress.pendingProps; const prevState = workInProgress.memoizedState; const prevChildren = prevState !== null ? prevState.element : null; - processUpdateQueue( - workInProgress, - updateQueue, - nextProps, - null, - renderExpirationTime, - ); + cloneUpdateQueue(current, workInProgress); + processUpdateQueue(workInProgress, nextProps, null, renderExpirationTime); const nextState = workInProgress.memoizedState; // Caution: React DevTools currently depends on this property // being called "element". @@ -1338,6 +1337,8 @@ function mountIndeterminateComponent( workInProgress.memoizedState = value.state !== null && value.state !== undefined ? value.state : null; + initializeUpdateQueue(workInProgress); + const getDerivedStateFromProps = Component.getDerivedStateFromProps; if (typeof getDerivedStateFromProps === 'function') { applyDerivedStateFromProps( diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 8dc7e45835ea5..5d0d05bf9e91f 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -9,6 +9,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {UpdateQueue} from './ReactUpdateQueue'; import React from 'react'; import {Update, Snapshot} from 'shared/ReactSideEffectTags'; @@ -38,6 +39,8 @@ import { createUpdate, ReplaceState, ForceUpdate, + initializeUpdateQueue, + cloneUpdateQueue, } from './ReactUpdateQueue'; import {NoWork} from './ReactFiberExpirationTime'; import { @@ -171,8 +174,9 @@ export function applyDerivedStateFromProps( // Once the update queue is empty, persist the derived state onto the // base state. - const updateQueue = workInProgress.updateQueue; - if (updateQueue !== null && workInProgress.expirationTime === NoWork) { + if (workInProgress.expirationTime === NoWork) { + // Queue is always non-null for classes + const updateQueue: UpdateQueue = (workInProgress.updateQueue: any); updateQueue.baseState = memoizedState; } } @@ -789,6 +793,8 @@ function mountClassInstance( instance.state = workInProgress.memoizedState; instance.refs = emptyRefsObject; + initializeUpdateQueue(workInProgress); + const contextType = ctor.contextType; if (typeof contextType === 'object' && contextType !== null) { instance.context = readContext(contextType); @@ -829,17 +835,8 @@ function mountClassInstance( } } - let updateQueue = workInProgress.updateQueue; - if (updateQueue !== null) { - processUpdateQueue( - workInProgress, - updateQueue, - newProps, - instance, - renderExpirationTime, - ); - instance.state = workInProgress.memoizedState; - } + processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime); + instance.state = workInProgress.memoizedState; const getDerivedStateFromProps = ctor.getDerivedStateFromProps; if (typeof getDerivedStateFromProps === 'function') { @@ -863,17 +860,13 @@ function mountClassInstance( callComponentWillMount(workInProgress, instance); // If we had additional state updates during this life-cycle, let's // process them now. - updateQueue = workInProgress.updateQueue; - if (updateQueue !== null) { - processUpdateQueue( - workInProgress, - updateQueue, - newProps, - instance, - renderExpirationTime, - ); - instance.state = workInProgress.memoizedState; - } + processUpdateQueue( + workInProgress, + newProps, + instance, + renderExpirationTime, + ); + instance.state = workInProgress.memoizedState; } if (typeof instance.componentDidMount === 'function') { @@ -936,17 +929,8 @@ function resumeMountClassInstance( const oldState = workInProgress.memoizedState; let newState = (instance.state = oldState); - let updateQueue = workInProgress.updateQueue; - if (updateQueue !== null) { - processUpdateQueue( - workInProgress, - updateQueue, - newProps, - instance, - renderExpirationTime, - ); - newState = workInProgress.memoizedState; - } + processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime); + newState = workInProgress.memoizedState; if ( oldProps === newProps && oldState === newState && @@ -1035,6 +1019,8 @@ function updateClassInstance( ): boolean { const instance = workInProgress.stateNode; + cloneUpdateQueue(current, workInProgress); + const oldProps = workInProgress.memoizedProps; instance.props = workInProgress.type === workInProgress.elementType @@ -1081,17 +1067,8 @@ function updateClassInstance( const oldState = workInProgress.memoizedState; let newState = (instance.state = oldState); - let updateQueue = workInProgress.updateQueue; - if (updateQueue !== null) { - processUpdateQueue( - workInProgress, - updateQueue, - newProps, - instance, - renderExpirationTime, - ); - newState = workInProgress.memoizedState; - } + processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime); + newState = workInProgress.memoizedState; if ( oldProps === newProps && diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 7a4c7dfae82b7..0a6c3a132cc2a 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/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 3f82c22f87871..f1314e3108013 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -25,6 +25,7 @@ import { } from 'shared/ReactFeatureFlags'; import {unstable_getThreadID} from 'scheduler/tracing'; import {NoPriority} from './SchedulerWithReactIntegration'; +import {initializeUpdateQueue} from './ReactUpdateQueue'; export type PendingInteractionMap = Map>; @@ -149,6 +150,8 @@ export function createFiberRoot( root.current = uninitializedFiber; uninitializedFiber.stateNode = root; + initializeUpdateQueue(uninitializedFiber); + return root; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 8273d92b3c7a9..649f37a410abd 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, @@ -2859,7 +2860,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 ( @@ -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; } diff --git a/packages/react-reconciler/src/ReactUpdateQueue.js b/packages/react-reconciler/src/ReactUpdateQueue.js index 0d2a8f68230bf..3cdf29b74efac 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.js @@ -89,13 +89,12 @@ 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, } from './ReactFiberNewContext'; import {Callback, ShouldCapture, DidCapture} from 'shared/ReactSideEffectTags'; -import {ClassComponent} from 'shared/ReactWorkTags'; import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags'; @@ -117,27 +116,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,41 +154,34 @@ if (__DEV__) { }; } -export function createUpdateQueue(baseState: State): UpdateQueue { +export function initializeUpdateQueue(fiber: Fiber): void { 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; + fiber.updateQueue = queue; } -function cloneUpdateQueue( - currentQueue: UpdateQueue, -): 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, - }; - return queue; +export function cloneUpdateQueue( + current: Fiber, + workInProgress: Fiber, +): void { + // Clone the update queue from current. Unless it's already a clone. + const queue: UpdateQueue = (workInProgress.updateQueue: any); + const currentQueue: UpdateQueue = (current.updateQueue: any); + if (queue === currentQueue) { + const clone: UpdateQueue = { + baseState: currentQueue.baseState, + baseQueue: currentQueue.baseQueue, + shared: currentQueue.shared, + effects: currentQueue.effects, + }; + workInProgress.updateQueue = clone; + } } export function createUpdate( @@ -210,90 +196,36 @@ 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); - } - } 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. - } - } + const updateQueue = fiber.updateQueue; + if (updateQueue === null) { + // Only occurs if the fiber has been unmounted. + return; } - if (queue2 === null || queue1 === queue2) { - // There's only a single queue. - appendUpdateToQueue(queue1, update); + + const 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,48 +244,25 @@ export function enqueueCapturedUpdate( workInProgress: Fiber, update: Update, ) { - // Captured updates go into a separate list, and only on the work-in- - // progress queue. - let workInProgressQueue = workInProgress.updateQueue; - if (workInProgressQueue === null) { - workInProgressQueue = workInProgress.updateQueue = createUpdateQueue( - workInProgress.memoizedState, - ); - } else { - // TODO: I put this here rather than createWorkInProgress so that we don't - // clone the queue unnecessarily. There's probably a better way to - // structure this. - workInProgressQueue = ensureWorkInProgressQueueIsAClone( - workInProgress, - workInProgressQueue, - ); + const current = workInProgress.alternate; + if (current !== null) { + // Ensure the work-in-progress queue is a clone + cloneUpdateQueue(current, workInProgress); } + // Captured updates go only on the work-in-progress queue. + const queue: UpdateQueue = (workInProgress.updateQueue: any); // 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 = queue.baseQueue; + if (last === null) { + queue.baseQueue = update.next = update; + update.next = update; } else { - workInProgressQueue.lastCapturedUpdate.next = update; - workInProgressQueue.lastCapturedUpdate = update; + update.next = last.next; + last.next = update; } } -function ensureWorkInProgressQueueIsAClone( - workInProgress: Fiber, - queue: UpdateQueue, -): UpdateQueue { - const current = workInProgress.alternate; - if (current !== null) { - // If the work-in-progress queue is equal to the current queue, - // we need to clone it first. - if (queue === current.updateQueue) { - queue = workInProgress.updateQueue = cloneUpdateQueue(queue); - } - } - return queue; -} - function getStateFromUpdate( workInProgress: Fiber, queue: UpdateQueue, @@ -429,158 +338,172 @@ function getStateFromUpdate( export function processUpdateQueue( workInProgress: Fiber, - queue: UpdateQueue, props: any, instance: any, renderExpirationTime: ExpirationTime, ): void { - hasForceUpdate = false; + // This is always non-null on a ClassComponent or HostRoot + const queue: UpdateQueue = (workInProgress.updateQueue: any); - queue = ensureWorkInProgressQueueIsAClone(workInProgress, queue); + hasForceUpdate = false; 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 +534,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 7ae9874105dce..7d199e2522d9d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -653,4 +653,176 @@ 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'); + }); + + 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 + // test failed because the update queue was initialized to the state of + // the alternate fiber. + let app; + class App extends React.Component { + state = {prevProp: 'A', count: 0}; + static getDerivedStateFromProps(props, state) { + // Add 100 whenever the label prop changes. The prev label is stored + // in state. If the state is dropped incorrectly, we'll fail to detect + // prop changes. + if (props.prop !== state.prevProp) { + return { + prevProp: props.prop, + count: state.count + 100, + }; + } + return null; + } + render() { + app = this; + return this.state.count; + } + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('0'); + + // Changing the prop causes the count to increase by 100 + await ReactNoop.act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('100'); + + // Now increment the count by 1 with a state update. And, in the same + // batch, change the prop back to its original value. + await ReactNoop.act(async () => { + root.render(); + app.setState(state => ({count: state.count + 1})); + }); + // There were two total prop changes, plus an increment. + expect(root).toMatchRenderedOutput('201'); + }); });