From 73a68fde48d2f34a73ece57376701ffc22ca55ec Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 29 Mar 2023 17:17:34 -0400 Subject: [PATCH] Move update processing into microtask When React receives new input (via `setState`, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happens `ensureRootIsScheduled`. If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution). So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for `flushSync`, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates. In production, this should have no observable behavior difference. In a testing environment that uses `act`, this should also not have a behavior difference because React will push these tasks to an internal `act` queue. However, tests that do not use `act` and do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to call `setState` outside of `act` and then immediately call `jest.runAllTimers()`, the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run. I don't expect this to be an issue in practice because most people do not write their tests this way. They either use `act`, or they write e2e-style tests. The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail. (There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.) The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event. --- .../src/__tests__/profilingCache-test.js | 2 +- ...MServerSelectiveHydration-test.internal.js | 4 +- .../react-reconciler/src/ReactFiberRoot.js | 1 + .../src/ReactFiberRootScheduler.js | 39 +-- .../src/ReactFiberSyncTaskQueue.js | 118 --------- .../src/ReactFiberWorkLoop.js | 242 +++--------------- .../src/ReactInternalTypes.js | 4 + .../ReactFlushSyncNoAggregateError-test.js | 13 +- .../__tests__/ReactIncrementalUpdates-test.js | 15 +- .../__tests__/ReactProfiler-test.internal.js | 8 +- 10 files changed, 59 insertions(+), 387 deletions(-) delete mode 100644 packages/react-reconciler/src/ReactFiberSyncTaskQueue.js diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js index d8d0690aa9c2a..a4f8613e61546 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js @@ -963,7 +963,7 @@ describe('ProfilingCache', () => { 2 => 0, }, "passiveEffectDuration": null, - "priorityLevel": "Immediate", + "priorityLevel": "Normal", "timestamp": 0, "updaters": [ { diff --git a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js index 6b51c9a189088..5f160bf709cab 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js @@ -803,9 +803,9 @@ describe('ReactDOMServerSelectiveHydration', () => { // boundary. See the next test below. assertLog([ 'D', - 'Clicked D', 'B', // Ideally this should be later. 'C', + 'Clicked D', 'Hover C', 'A', ]); @@ -963,10 +963,10 @@ describe('ReactDOMServerSelectiveHydration', () => { // boundary. See the next test below. assertLog([ 'D', - 'Clicked D', 'B', // Ideally this should be later. 'C', // Capture phase isn't replayed + 'Clicked D', // Mouseout isn't replayed 'Mouse Over C', 'Mouse Enter C', diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index c8df0a9e14673..2733d0ccf3515 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -63,6 +63,7 @@ function FiberRootNode( this.cancelPendingCommit = null; this.context = null; this.pendingContext = null; + this.next = null; this.callbackNode = null; this.callbackPriority = NoLane; this.eventTimes = createLaneMap(NoLanes); diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 5e6a62b82952c..61979266ce121 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -11,10 +11,10 @@ import type {FiberRoot} from './ReactInternalTypes'; import type {Lane} from './ReactFiberLane'; import type {PriorityLevel} from 'scheduler/src/SchedulerPriorities'; -import {enableDeferRootSchedulingToMicrotask} from 'shared/ReactFeatureFlags'; import { NoLane, NoLanes, + SyncLane, getHighestPriorityLane, getNextLanes, includesOnlyNonUrgentLanes, @@ -111,14 +111,6 @@ export function ensureRootIsScheduled(root: FiberRoot): void { scheduleImmediateTask(processRootScheduleInMicrotask); } } - - if (!enableDeferRootSchedulingToMicrotask) { - // While this flag is disabled, we schedule the task immediately instead - // of waiting for the microtask to fire. - // TODO: We need to land enableDeferRootSchedulingToMicrotask ASAP to - // unblock additional features we have planned. - scheduleTaskForRootDuringMicrotask(root, now()); - } } // TODO: Rename to something else. This isn't a generic callback queue anymore. @@ -255,7 +247,7 @@ function processRootScheduleInMicrotask() { } else { // This root still has work. Keep it in the list. prev = root; - if (includesSyncLane) { + if (includesSyncLane(nextLanes)) { mightHavePendingSyncWork = true; } } @@ -275,9 +267,6 @@ function scheduleTaskForRootDuringMicrotask( // rendering task right before we yield to the main thread. It should never be // called synchronously. // - // TODO: Unless enableDeferRootSchedulingToMicrotask is off. We need to land - // that ASAP to unblock additional features we have planned. - // // This function also never performs React work synchronously; it should // only schedule work to be performed later, in a separate task or microtask. @@ -306,30 +295,25 @@ function scheduleTaskForRootDuringMicrotask( ) { // Fast path: There's nothing to work on. if (existingCallbackNode !== null) { - if (__DEV__ && existingCallbackNode === fakeActCallbackNode) { - // Special `act` case: check if this is the fake callback node used by - // the `act` implementation. - } else { - Scheduler_cancelCallback(existingCallbackNode); - } + cancelCallback(existingCallbackNode); } root.callbackNode = null; root.callbackPriority = NoLane; return NoLane; } - // We use the highest priority lane to represent the priority of the callback. - const existingCallbackPriority = root.callbackPriority; - const newCallbackPriority = getHighestPriorityLane(nextLanes); - // Schedule a new callback in the host environment. - if (includesSyncLane(newCallbackPriority)) { + if (includesSyncLane(nextLanes)) { // Synchronous work will be flushed at the end of the microtask; we don't // need to schedule anything extra. - mightHavePendingSyncWork = true; - root.callbackPriority = newCallbackPriority; + root.callbackPriority = SyncLane; root.callbackNode = null; + return SyncLane; } else { + // We use the highest priority lane to represent the priority of the callback. + const existingCallbackPriority = root.callbackPriority; + const newCallbackPriority = getHighestPriorityLane(nextLanes); + if ( newCallbackPriority === existingCallbackPriority && // Special case related to `act`. If the currently scheduled task is a @@ -374,9 +358,8 @@ function scheduleTaskForRootDuringMicrotask( root.callbackPriority = newCallbackPriority; root.callbackNode = newCallbackNode; + return newCallbackPriority; } - - return newCallbackPriority; } export type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null; diff --git a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js deleted file mode 100644 index 0b11be38a1dac..0000000000000 --- a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js +++ /dev/null @@ -1,118 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import type {SchedulerCallback} from './Scheduler'; - -import { - DiscreteEventPriority, - getCurrentUpdatePriority, - setCurrentUpdatePriority, -} from './ReactEventPriorities'; -import {ImmediatePriority, scheduleCallback} from './Scheduler'; - -let syncQueue: Array | null = null; -let includesLegacySyncCallbacks: boolean = false; -let isFlushingSyncQueue: boolean = false; - -export function scheduleSyncCallback(callback: SchedulerCallback) { - // Push this callback into an internal queue. We'll flush these either in - // the next tick, or earlier if something calls `flushSyncCallbackQueue`. - if (syncQueue === null) { - syncQueue = [callback]; - } else { - // Push onto existing queue. Don't need to schedule a callback because - // we already scheduled one when we created the queue. - syncQueue.push(callback); - } -} - -export function scheduleLegacySyncCallback(callback: SchedulerCallback) { - includesLegacySyncCallbacks = true; - scheduleSyncCallback(callback); -} - -export function flushSyncCallbacksOnlyInLegacyMode() { - // Only flushes the queue if there's a legacy sync callback scheduled. - // TODO: There's only a single type of callback: performSyncOnWorkOnRoot. So - // it might make more sense for the queue to be a list of roots instead of a - // list of generic callbacks. Then we can have two: one for legacy roots, one - // for concurrent roots. And this method would only flush the legacy ones. - if (includesLegacySyncCallbacks) { - flushSyncCallbacks(); - } -} - -export function flushSyncCallbacks(): null { - if (!isFlushingSyncQueue && syncQueue !== null) { - // Prevent re-entrance. - isFlushingSyncQueue = true; - - // Set the event priority to discrete - // TODO: Is this necessary anymore? The only user code that runs in this - // queue is in the render or commit phases, which already set the - // event priority. Should be able to remove. - const previousUpdatePriority = getCurrentUpdatePriority(); - setCurrentUpdatePriority(DiscreteEventPriority); - - let errors: Array | null = null; - - const queue = syncQueue; - // $FlowFixMe[incompatible-use] found when upgrading Flow - for (let i = 0; i < queue.length; i++) { - // $FlowFixMe[incompatible-use] found when upgrading Flow - let callback: SchedulerCallback = queue[i]; - try { - do { - const isSync = true; - // $FlowFixMe[incompatible-type] we bail out when we get a null - callback = callback(isSync); - } while (callback !== null); - } catch (error) { - // Collect errors so we can rethrow them at the end - if (errors === null) { - errors = [error]; - } else { - errors.push(error); - } - } - } - - syncQueue = null; - includesLegacySyncCallbacks = false; - setCurrentUpdatePriority(previousUpdatePriority); - isFlushingSyncQueue = false; - - if (errors !== null) { - if (errors.length > 1) { - if (typeof AggregateError === 'function') { - // eslint-disable-next-line no-undef - throw new AggregateError(errors); - } else { - for (let i = 1; i < errors.length; i++) { - scheduleCallback( - ImmediatePriority, - throwError.bind(null, errors[i]), - ); - } - const firstError = errors[0]; - throw firstError; - } - } else { - const error = errors[0]; - throw error; - } - } - } - - return null; -} - -function throwError(error: mixed) { - throw error; -} diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index b287e279211cd..f32d13a176b63 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -22,6 +22,7 @@ import type { TransitionAbort, } from './ReactFiberTracingMarkerComponent'; import type {OffscreenInstance} from './ReactFiberOffscreenComponent'; +import type {RenderTaskFn} from './ReactFiberRootScheduler'; import { replayFailedUnitOfWorkWithInvokeGuardedCallback, @@ -46,21 +47,12 @@ import is from 'shared/objectIs'; import { // Aliased because `act` will override and push to an internal queue scheduleCallback as Scheduler_scheduleCallback, - cancelCallback as Scheduler_cancelCallback, shouldYield, requestPaint, now, - ImmediatePriority as ImmediateSchedulerPriority, - UserBlockingPriority as UserBlockingSchedulerPriority, NormalPriority as NormalSchedulerPriority, IdlePriority as IdleSchedulerPriority, } from './Scheduler'; -import { - flushSyncCallbacks, - flushSyncCallbacksOnlyInLegacyMode, - scheduleSyncCallback, - scheduleLegacySyncCallback, -} from './ReactFiberSyncTaskQueue'; import { logCommitStarted, logCommitStopped, @@ -79,9 +71,7 @@ import { noTimeout, afterActiveInstanceBlur, getCurrentEventPriority, - supportsMicrotasks, errorHydratingContainer, - scheduleMicrotask, prepareRendererToRender, resetRendererAfterRender, startSuspendingCommit, @@ -154,7 +144,6 @@ import { includesBlockingLane, includesExpiredLane, getNextLanes, - markStarvedLanesAsExpired, getLanesToRetrySynchronouslyOnError, getMostRecentEventTime, markRootUpdated, @@ -162,7 +151,6 @@ import { markRootPinged, markRootEntangled, markRootFinished, - getHighestPriorityLane, addFiberToLanesMap, movePendingFibersToMemoized, addTransitionToLanesMap, @@ -173,9 +161,7 @@ import { } from './ReactFiberLane'; import { DiscreteEventPriority, - ContinuousEventPriority, DefaultEventPriority, - IdleEventPriority, getCurrentUpdatePriority, setCurrentUpdatePriority, lowerEventPriority, @@ -290,6 +276,12 @@ import { } from './ReactFiberSuspenseContext'; import {resolveDefaultProps} from './ReactFiberLazyComponent'; import {resetChildReconcilerOnUnwind} from './ReactChildFiber'; +import { + ensureRootIsScheduled, + flushSyncCallbacks, + flushSyncCallbacksOnlyInLegacyMode, + getContinuationForRoot, +} from './ReactFiberRootScheduler'; const ceil = Math.ceil; @@ -610,6 +602,10 @@ export function getWorkInProgressRootRenderLanes(): Lanes { return workInProgressRootRenderLanes; } +export function isWorkLoopSuspendedOnData(): boolean { + return workInProgressSuspendedReason === SuspendedOnData; +} + export function requestEventTime(): number { if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { // We're inside React, so it's fine to read the actual time. @@ -820,7 +816,7 @@ export function scheduleUpdateOnFiber( } } - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); if ( lane === SyncLane && executionContext === NoContext && @@ -859,7 +855,7 @@ export function scheduleInitialHydrationOnRoot( const current = root.current; current.lanes = lane; markRootUpdated(root, lane, eventTime); - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); } export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean { @@ -868,169 +864,12 @@ export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean { return (executionContext & RenderContext) !== NoContext; } -// Use this function to schedule a task for a root. There's only one task per -// root; if a task was already scheduled, we'll check to make sure the priority -// of the existing task is the same as the priority of the next level that the -// root has work on. This function is called on every update, and right before -// exiting a task. -function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { - const existingCallbackNode = root.callbackNode; - - // Check if any lanes are being starved by other work. If so, mark them as - // expired so we know to work on those next. - markStarvedLanesAsExpired(root, currentTime); - - // Determine the next lanes to work on, and their priority. - const nextLanes = getNextLanes( - root, - root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, - ); - - if (nextLanes === NoLanes) { - // Special case: There's nothing to work on. - if (existingCallbackNode !== null) { - cancelCallback(existingCallbackNode); - } - root.callbackNode = null; - root.callbackPriority = NoLane; - return; - } - - // If this root is currently suspended and waiting for data to resolve, don't - // schedule a task to render it. We'll either wait for a ping, or wait to - // receive an update. - if ( - workInProgressSuspendedReason === SuspendedOnData && - workInProgressRoot === root - ) { - root.callbackPriority = NoLane; - root.callbackNode = null; - return; - } - - const cancelPendingCommit = root.cancelPendingCommit; - if (cancelPendingCommit !== null) { - // We should only interrupt a pending commit if the new update - // is urgent. - if (includesOnlyNonUrgentLanes(nextLanes)) { - // The new update is not urgent. Don't interrupt the pending commit. - root.callbackPriority = NoLane; - root.callbackNode = null; - return; - } - } - - // We use the highest priority lane to represent the priority of the callback. - const newCallbackPriority = getHighestPriorityLane(nextLanes); - - // Check if there's an existing task. We may be able to reuse it. - const existingCallbackPriority = root.callbackPriority; - if ( - existingCallbackPriority === newCallbackPriority && - // Special case related to `act`. If the currently scheduled task is a - // Scheduler task, rather than an `act` task, cancel it and re-scheduled - // on the `act` queue. - !( - __DEV__ && - ReactCurrentActQueue.current !== null && - existingCallbackNode !== fakeActCallbackNode - ) - ) { - if (__DEV__) { - // If we're going to re-use an existing task, it needs to exist. - // Assume that discrete update microtasks are non-cancellable and null. - // TODO: Temporary until we confirm this warning is not fired. - if ( - existingCallbackNode == null && - !includesSyncLane(existingCallbackPriority) - ) { - console.error( - 'Expected scheduled callback to exist. This error is likely caused by a bug in React. Please file an issue.', - ); - } - } - // The priority hasn't changed. We can reuse the existing task. Exit. - return; - } - - if (existingCallbackNode != null) { - // Cancel the existing callback. We'll schedule a new one below. - cancelCallback(existingCallbackNode); - } - - // Schedule a new callback. - let newCallbackNode; - if (includesSyncLane(newCallbackPriority)) { - // Special case: Sync React callbacks are scheduled on a special - // internal queue - if (root.tag === LegacyRoot) { - scheduleLegacySyncCallback(performSyncWorkOnRoot.bind(null, root)); - } else { - scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); - } - if (supportsMicrotasks) { - // Flush the queue in a microtask. - if (__DEV__ && ReactCurrentActQueue.current !== null) { - // Inside `act`, use our internal `act` queue so that these get flushed - // at the end of the current scope even when using the sync version - // of `act`. - ReactCurrentActQueue.current.push(flushSyncCallbacks); - } else { - scheduleMicrotask(() => { - // In Safari, appending an iframe forces microtasks to run. - // https://github.com/facebook/react/issues/22459 - // We don't support running callbacks in the middle of render - // or commit so we need to check against that. - if ( - (executionContext & (RenderContext | CommitContext)) === - NoContext - ) { - // Note that this would still prematurely flush the callbacks - // if this happens outside render or commit phase (e.g. in an event). - flushSyncCallbacks(); - } - }); - } - } else { - // Flush the queue in an Immediate task. - scheduleCallback(ImmediateSchedulerPriority, flushSyncCallbacks); - } - newCallbackNode = null; - } else { - let schedulerPriorityLevel; - switch (lanesToEventPriority(nextLanes)) { - case DiscreteEventPriority: - schedulerPriorityLevel = ImmediateSchedulerPriority; - break; - case ContinuousEventPriority: - schedulerPriorityLevel = UserBlockingSchedulerPriority; - break; - case DefaultEventPriority: - schedulerPriorityLevel = NormalSchedulerPriority; - break; - case IdleEventPriority: - schedulerPriorityLevel = IdleSchedulerPriority; - break; - default: - schedulerPriorityLevel = NormalSchedulerPriority; - break; - } - newCallbackNode = scheduleCallback( - schedulerPriorityLevel, - performConcurrentWorkOnRoot.bind(null, root), - ); - } - - root.callbackPriority = newCallbackPriority; - root.callbackNode = newCallbackNode; -} - // This is the entry point for every concurrent task, i.e. anything that // goes through Scheduler. -function performConcurrentWorkOnRoot( +export function performConcurrentWorkOnRoot( root: FiberRoot, didTimeout: boolean, -): $FlowFixMe { +): RenderTaskFn | null { if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { resetNestedUpdateFlag(); } @@ -1063,6 +902,7 @@ function performConcurrentWorkOnRoot( // Determine the next lanes to work on, using the fields stored // on the root. + // TODO: This was already computed in the caller. Pass it as an argument. let lanes = getNextLanes( root, root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, @@ -1109,7 +949,7 @@ function performConcurrentWorkOnRoot( const fatalError = workInProgressRootFatalError; prepareFreshStack(root, NoLanes); markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); throw fatalError; } @@ -1158,7 +998,7 @@ function performConcurrentWorkOnRoot( const fatalError = workInProgressRootFatalError; prepareFreshStack(root, NoLanes); markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); throw fatalError; } @@ -1174,13 +1014,8 @@ function performConcurrentWorkOnRoot( } } - ensureRootIsScheduled(root, now()); - if (root.callbackNode === originalCallbackNode) { - // The task node scheduled for this root is the same one that's - // currently executed. Need to return a continuation. - return performConcurrentWorkOnRoot.bind(null, root); - } - return null; + ensureRootIsScheduled(root); + return getContinuationForRoot(root, originalCallbackNode); } function recoverFromConcurrentError( @@ -1532,7 +1367,7 @@ function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) { // This is the entry point for synchronous tasks that don't go // through Scheduler -function performSyncWorkOnRoot(root: FiberRoot) { +export function performSyncWorkOnRoot(root: FiberRoot): null { if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { syncNestedUpdateFlag(); } @@ -1543,10 +1378,11 @@ function performSyncWorkOnRoot(root: FiberRoot) { flushPassiveEffects(); + // TODO: This was already computed in the caller. Pass it as an argument. let lanes = getNextLanes(root, NoLanes); if (!includesSyncLane(lanes)) { // There's no remaining sync work left. - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); return null; } @@ -1575,7 +1411,7 @@ function performSyncWorkOnRoot(root: FiberRoot) { const fatalError = workInProgressRootFatalError; prepareFreshStack(root, NoLanes); markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); throw fatalError; } @@ -1584,7 +1420,7 @@ function performSyncWorkOnRoot(root: FiberRoot) { // cases where need to exit the current render without producing a // consistent tree or committing. markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); return null; } @@ -1601,7 +1437,7 @@ function performSyncWorkOnRoot(root: FiberRoot) { // Before exiting, make sure there's a callback scheduled for the next // pending level. - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); return null; } @@ -1609,9 +1445,12 @@ function performSyncWorkOnRoot(root: FiberRoot) { export function flushRoot(root: FiberRoot, lanes: Lanes) { if (lanes !== NoLanes) { markRootEntangled(root, mergeLanes(lanes, SyncLane)); - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); + // TODO: For historical reasons this flushes all sync work across all + // roots. It shouldn't really matter either way, but we could change this + // to only flush the given root. flushSyncCallbacks(); } } @@ -2314,7 +2153,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // Ensure the root is scheduled. We should do this even if we're // currently working on a different root, so that we resume // rendering later. - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); }; thenable.then(onResolution, onResolution); break outer; @@ -3151,7 +2990,7 @@ function commitRootImpl( // Always call this before exiting `commitRoot`, to ensure that any // additional work on this root is scheduled. - ensureRootIsScheduled(root, now()); + ensureRootIsScheduled(root); if (recoverableErrors !== null) { // There were errors during this render, but recovered from them without @@ -3497,7 +3336,7 @@ function captureCommitPhaseErrorOnRoot( const eventTime = requestEventTime(); if (root !== null) { markRootUpdated(root, SyncLane, eventTime); - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); } } @@ -3546,7 +3385,7 @@ export function captureCommitPhaseError( const eventTime = requestEventTime(); if (root !== null) { markRootUpdated(root, SyncLane, eventTime); - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); } return; } @@ -3629,7 +3468,6 @@ function pingSuspendedRoot( pingCache.delete(wakeable); } - const eventTime = requestEventTime(); markRootPinged(root, pingedLanes); warnIfSuspenseResolutionNotWrappedWithActDEV(root); @@ -3672,7 +3510,7 @@ function pingSuspendedRoot( } } - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); } function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) { @@ -3690,7 +3528,7 @@ function retryTimedOutBoundary(boundaryFiber: Fiber, retryLane: Lane) { const root = enqueueConcurrentRenderForLane(boundaryFiber, retryLane); if (root !== null) { markRootUpdated(root, retryLane, eventTime); - ensureRootIsScheduled(root, eventTime); + ensureRootIsScheduled(root); } } @@ -4172,14 +4010,6 @@ function scheduleCallback(priorityLevel: any, callback) { } } -function cancelCallback(callbackNode: any) { - if (__DEV__ && callbackNode === fakeActCallbackNode) { - return; - } - // In production, always call Scheduler. This function will be stripped out. - return Scheduler_cancelCallback(callbackNode); -} - function shouldForceFlushFallbacksInDEV() { // Never force flush in production. This function should get stripped out. return __DEV__ && ReactCurrentActQueue.current !== null; diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index a672b9669abcf..ae1e75bfe9467 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -240,6 +240,10 @@ type BaseFiberRootProperties = { MutableSource | MutableSourceVersion, > | null, + // Used to create a linked list that represent all the roots that have + // pending work scheduled on them. + next: FiberRoot | null, + // Node returned by Scheduler.scheduleCallback. Represents the next rendering // task that the root will work on. callbackNode: any, diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js index 129b79f743144..8c1bc7475be36 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js @@ -3,7 +3,6 @@ let ReactNoop; let Scheduler; let act; let assertLog; -let waitForThrow; let overrideQueueMicrotask; let flushFakeMicrotasks; @@ -43,7 +42,6 @@ describe('ReactFlushSync (AggregateError not available)', () => { const InternalTestUtils = require('internal-test-utils'); assertLog = InternalTestUtils.assertLog; - waitForThrow = InternalTestUtils.waitForThrow; }); function Text({text}) { @@ -95,15 +93,6 @@ describe('ReactFlushSync (AggregateError not available)', () => { // AggregateError is not available, React throws the first error, then // throws the remaining errors in separate tasks. expect(error).toBe(aahh); - - // TODO: Currently the remaining error is rethrown in an Immediate Scheduler - // task, but this may change to a timer or microtask in the future. The - // exact mechanism is an implementation detail; they just need to be logged - // in the order the occurred. - - // This will start throwing if we change it to rethrow in a microtask. - flushFakeMicrotasks(); - - await waitForThrow(nooo); + expect(flushFakeMicrotasks).toThrow(nooo); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 7413e51a3ab14..b224e4d776482 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -552,19 +552,8 @@ describe('ReactIncrementalUpdates', () => { // The transition should not have expired, so we should be able to // partially render it. await waitFor(['A']); - - // FIXME: We should be able to partially render B, too, but currently it - // expires. This is an existing bug that I discovered, which will be fixed - // in a PR that I'm currently working on. - // - // Correct behavior: - // await waitFor(['B']); - // await waitForAll(['C', 'D']); - // - // Current behavior: - await waitFor(['B'], { - additionalLogsAfterAttemptingToYield: ['C', 'D'], - }); + await waitFor(['B']); + await waitForAll(['C', 'D']); }); it('regression: does not expire soon due to previous expired work', async () => { diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 3602a0190be65..e4fd2a323b3e0 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -258,13 +258,7 @@ describe(`onRender`, () => { // TODO: unstable_now is called by more places than just the profiler. // Rewrite this test so it's less fragile. - assertLog([ - 'read current time', - 'read current time', - 'read current time', - 'read current time', - 'read current time', - ]); + assertLog(['read current time', 'read current time', 'read current time']); // Restore original mock jest.mock('scheduler', () => jest.requireActual('scheduler/unstable_mock'));