diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 15730d3e5b624..9f267e8345e39 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -15,6 +15,9 @@ import type {BatchConfigTransition} from './ReactFiberTracingMarkerComponent'; import { disableLegacyMode, enableDeferRootSchedulingToMicrotask, + disableSchedulerTimeoutInWorkLoop, + enableProfilerTimer, + enableProfilerNestedUpdatePhase, } from 'shared/ReactFeatureFlags'; import { NoLane, @@ -31,12 +34,12 @@ import { CommitContext, NoContext, RenderContext, + flushPassiveEffects, getExecutionContext, getWorkInProgressRoot, getWorkInProgressRootRenderLanes, isWorkLoopSuspendedOnData, - performConcurrentWorkOnRoot, - performSyncWorkOnRoot, + performWorkOnRoot, } from './ReactFiberWorkLoop'; import {LegacyRoot} from './ReactRootTags'; import { @@ -62,6 +65,10 @@ import { } from './ReactFiberConfig'; import ReactSharedInternals from 'shared/ReactSharedInternals'; +import { + resetNestedUpdateFlag, + syncNestedUpdateFlag, +} from './ReactProfilerTimer'; // A linked list of all the roots with pending work. In an idiomatic app, // there's only a single root, but we do support multi root apps, hence this @@ -387,7 +394,7 @@ function scheduleTaskForRootDuringMicrotask( const newCallbackNode = scheduleCallback( schedulerPriorityLevel, - performConcurrentWorkOnRoot.bind(null, root), + performWorkOnRootViaSchedulerTask.bind(null, root), ); root.callbackPriority = newCallbackPriority; @@ -396,15 +403,67 @@ function scheduleTaskForRootDuringMicrotask( } } -export type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null; +type RenderTaskFn = (didTimeout: boolean) => RenderTaskFn | null; -export function getContinuationForRoot( +function performWorkOnRootViaSchedulerTask( root: FiberRoot, - originalCallbackNode: mixed, + didTimeout: boolean, ): RenderTaskFn | null { - // This is called at the end of `performConcurrentWorkOnRoot` to determine - // if we need to schedule a continuation task. - // + // This is the entry point for concurrent tasks scheduled via Scheduler (and + // postTask, in the future). + + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { + resetNestedUpdateFlag(); + } + + // Flush any pending passive effects before deciding which lanes to work on, + // in case they schedule additional work. + const originalCallbackNode = root.callbackNode; + const didFlushPassiveEffects = flushPassiveEffects(); + if (didFlushPassiveEffects) { + // Something in the passive effect phase may have canceled the current task. + // Check if the task node for this root was changed. + if (root.callbackNode !== originalCallbackNode) { + // The current task was canceled. Exit. We don't need to call + // `ensureRootIsScheduled` because the check above implies either that + // there's a new task, or that there's no remaining work on this root. + return null; + } else { + // Current task was not canceled. Continue. + } + } + + // Determine the next lanes to work on, using the fields stored on the root. + // TODO: We already called getNextLanes when we scheduled the callback; we + // should be able to avoid calling it again by stashing the result on the + // root object. However, because we always schedule the callback during + // a microtask (scheduleTaskForRootDuringMicrotask), it's possible that + // an update was scheduled earlier during this same browser task (and + // therefore before the microtasks have run). That's because Scheduler batches + // together multiple callbacks into a single browser macrotask, without + // yielding to microtasks in between. We should probably change this to align + // with the postTask behavior (and literally use postTask when + // it's available). + const workInProgressRoot = getWorkInProgressRoot(); + const workInProgressRootRenderLanes = getWorkInProgressRootRenderLanes(); + const lanes = getNextLanes( + root, + root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes, + ); + if (lanes === NoLanes) { + // No more work on this root. + return null; + } + + // Enter the work loop. + // TODO: We only check `didTimeout` defensively, to account for a Scheduler + // bug we're still investigating. Once the bug in Scheduler is fixed, + // we can remove this, since we track expiration ourselves. + const forceSync = !disableSchedulerTimeoutInWorkLoop && didTimeout; + performWorkOnRoot(root, lanes, forceSync); + + // The work loop yielded, but there may or may not be work left at the current + // priority. Need to determine whether we need to schedule a continuation. // Usually `scheduleTaskForRootDuringMicrotask` only runs inside a microtask; // however, since most of the logic for determining if we need a continuation // versus a new task is the same, we cheat a bit and call it here. This is @@ -414,11 +473,27 @@ export function getContinuationForRoot( 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 performWorkOnRootViaSchedulerTask.bind(null, root); } return null; } +function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) { + // This is the entry point for synchronous tasks that don't go + // through Scheduler. + const didFlushPassiveEffects = flushPassiveEffects(); + if (didFlushPassiveEffects) { + // If passive effects were flushed, exit to the outer work loop in the root + // scheduler, so we can recompute the priority. + return null; + } + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { + syncNestedUpdateFlag(); + } + const forceSync = true; + performWorkOnRoot(root, lanes, forceSync); +} + const fakeActCallbackNode = {}; function scheduleCallback( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index a0df584423e13..ba3270214c6a5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -22,7 +22,6 @@ import type { TransitionAbort, } from './ReactFiberTracingMarkerComponent'; import type {OffscreenInstance} from './ReactFiberActivityComponent'; -import type {RenderTaskFn} from './ReactFiberRootScheduler'; import type {Resource} from './ReactFiberConfig'; import { @@ -32,7 +31,6 @@ import { enableProfilerNestedUpdatePhase, enableDebugTracing, enableSchedulingProfiler, - disableSchedulerTimeoutInWorkLoop, enableUpdaterTracking, enableCache, enableTransitionTracing, @@ -250,11 +248,9 @@ import { recordRenderTime, recordCommitTime, recordCommitEndTime, - resetNestedUpdateFlag, startProfilerTimer, stopProfilerTimerIfRunningAndRecordDuration, stopProfilerTimerIfRunningAndRecordIncompleteDuration, - syncNestedUpdateFlag, } from './ReactProfilerTimer'; import {setCurrentTrackFromLanes} from './ReactFiberPerformanceTrack'; @@ -308,7 +304,6 @@ import { ensureRootIsScheduled, flushSyncWorkOnAllRoots, flushSyncWorkOnLegacyRootsOnly, - getContinuationForRoot, requestTransitionLane, } from './ReactFiberRootScheduler'; import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext'; @@ -890,59 +885,22 @@ export function isUnsafeClassRenderPhaseUpdate(fiber: Fiber): boolean { return (executionContext & RenderContext) !== NoContext; } -// This is the entry point for every concurrent task, i.e. anything that -// goes through Scheduler. -export function performConcurrentWorkOnRoot( +export function performWorkOnRoot( root: FiberRoot, - didTimeout: boolean, -): RenderTaskFn | null { - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { - resetNestedUpdateFlag(); - } - + lanes: Lanes, + forceSync: boolean, +): void { if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { throw new Error('Should not already be working.'); } - // Flush any pending passive effects before deciding which lanes to work on, - // in case they schedule additional work. - const originalCallbackNode = root.callbackNode; - const didFlushPassiveEffects = flushPassiveEffects(); - if (didFlushPassiveEffects) { - // Something in the passive effect phase may have canceled the current task. - // Check if the task node for this root was changed. - if (root.callbackNode !== originalCallbackNode) { - // The current task was canceled. Exit. We don't need to call - // `ensureRootIsScheduled` because the check above implies either that - // there's a new task, or that there's no remaining work on this root. - return null; - } else { - // Current task was not canceled. Continue. - } - } - - // 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, - ); - if (lanes === NoLanes) { - // Defensive coding. This is never expected to happen. - return null; - } - // We disable time-slicing in some cases: if the work has been CPU-bound // for too long ("expired" work, to prevent starvation), or we're in // sync-updates-by-default mode. - // TODO: We only check `didTimeout` defensively, to account for a Scheduler - // bug we're still investigating. Once the bug in Scheduler is fixed, - // we can remove this, since we track expiration ourselves. const shouldTimeSlice = + !forceSync && !includesBlockingLane(lanes) && - !includesExpiredLane(root, lanes) && - (disableSchedulerTimeoutInWorkLoop || !didTimeout); + !includesExpiredLane(root, lanes); let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes); @@ -984,7 +942,10 @@ export function performConcurrentWorkOnRoot( } // Check if something threw - if (exitStatus === RootErrored) { + if ( + (disableLegacyMode || root.tag !== LegacyRoot) && + exitStatus === RootErrored + ) { const lanesThatJustErrored = lanes; const errorRetryLanes = getLanesToRetrySynchronouslyOnError( root, @@ -1033,7 +994,6 @@ export function performConcurrentWorkOnRoot( } ensureRootIsScheduled(root); - return getContinuationForRoot(root, originalCallbackNode); } function recoverFromConcurrentError( @@ -1464,104 +1424,6 @@ function markRootSuspended( ); } -// This is the entry point for synchronous tasks that don't go -// through Scheduler -export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null { - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { - throw new Error('Should not already be working.'); - } - - const didFlushPassiveEffects = flushPassiveEffects(); - if (didFlushPassiveEffects) { - // If passive effects were flushed, exit to the outer work loop in the root - // scheduler, so we can recompute the priority. - // TODO: We don't actually need this `ensureRootIsScheduled` call because - // this path is only reachable if the root is already part of the schedule. - // I'm including it only for consistency with the other exit points from - // this function. Can address in a subsequent refactor. - ensureRootIsScheduled(root); - return null; - } - - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { - syncNestedUpdateFlag(); - } - - let exitStatus = renderRootSync(root, lanes); - if ( - (disableLegacyMode || root.tag !== LegacyRoot) && - exitStatus === RootErrored - ) { - // If something threw an error, try rendering one more time. We'll render - // synchronously to block concurrent data mutations, and we'll includes - // all pending updates are included. If it still fails after the second - // attempt, we'll give up and commit the resulting tree. - const originallyAttemptedLanes = lanes; - const errorRetryLanes = getLanesToRetrySynchronouslyOnError( - root, - originallyAttemptedLanes, - ); - if (errorRetryLanes !== NoLanes) { - lanes = errorRetryLanes; - exitStatus = recoverFromConcurrentError( - root, - originallyAttemptedLanes, - errorRetryLanes, - ); - } - } - - if (exitStatus === RootFatalErrored) { - prepareFreshStack(root, NoLanes); - markRootSuspended(root, lanes, NoLane, false); - ensureRootIsScheduled(root); - return null; - } - - if (exitStatus === RootDidNotComplete) { - // The render unwound without completing the tree. This happens in special - // cases where need to exit the current render without producing a - // consistent tree or committing. - markRootSuspended( - root, - lanes, - workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, - ); - ensureRootIsScheduled(root); - return null; - } - - let renderEndTime = 0; - if (enableProfilerTimer && enableComponentPerformanceTrack) { - renderEndTime = now(); - } - - // We now have a consistent tree. Because this is a sync render, we - // will commit it even if something suspended. - const finishedWork: Fiber = (root.current.alternate: any); - root.finishedWork = finishedWork; - root.finishedLanes = lanes; - commitRoot( - root, - workInProgressRootRecoverableErrors, - workInProgressTransitions, - workInProgressRootDidIncludeRecursiveRenderUpdate, - workInProgressDeferredLane, - workInProgressRootInterleavedUpdatedLanes, - workInProgressSuspendedRetryLanes, - IMMEDIATE_COMMIT, - renderStartTime, - renderEndTime, - ); - - // Before exiting, make sure there's a callback scheduled for the next - // pending level. - ensureRootIsScheduled(root); - - return null; -} - export function flushRoot(root: FiberRoot, lanes: Lanes) { if (lanes !== NoLanes) { upgradePendingLanesToSync(root, lanes); diff --git a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js index 6c24fdc980285..37d907b0fdc65 100644 --- a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js @@ -349,6 +349,7 @@ describe('ReactSiblingPrerendering', () => {
}> +
@@ -370,10 +371,17 @@ describe('ReactSiblingPrerendering', () => { // is throttled because it's been less than a Just Noticeable Difference // since the outer fallback was committed. // - // In the meantime, we could choose to start prerendering B, but instead + // In the meantime, we could choose to start prerendering C, but instead // we wait for a JND to elapse and the commit to finish — it's not // worth discarding the work we've already done. - await waitForAll(['A', 'Suspend! [B]', 'Loading inner...']); + await waitForAll([ + 'A', + 'Suspend! [B]', + + // C is skipped because we're no longer in prerendering mode; there's + // a new fallback we can show. + 'Loading inner...', + ]); expect(root).toMatchRenderedOutput(
Loading outer...
); // Fire the timer to commit the outer fallback. @@ -385,8 +393,10 @@ describe('ReactSiblingPrerendering', () => { , ); }); - // Once the outer fallback is committed, we can start prerendering B. - assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [B]'] : []); + // Once the inner fallback is committed, we can start prerendering C. + assertLog( + gate('enableSiblingPrerendering') ? ['Suspend! [B]', 'Suspend! [C]'] : [], + ); }); it( diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js index 972416eddedb8..4dbba1bca21f0 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js @@ -239,11 +239,7 @@ describe('ReactSuspenseyCommitPhase', () => { expect(root).toMatchRenderedOutput(); }); - // @TODO This isn't actually ideal behavior. We would really want the commit to suspend - // even if it is forced to be sync because we don't want to FOUC but refactoring the sync - // pathway is too risky to land right now so we just accept that we can still FOUC in this - // very specific case. - it('does not suspend commit during urgent initial mount at the root when sync rendering', async () => { + it('does suspend commit during urgent initial mount at the root when sync rendering', async () => { const root = ReactNoop.createRoot(); await act(async () => { ReactNoop.flushSync(() => { @@ -252,19 +248,15 @@ describe('ReactSuspenseyCommitPhase', () => { }); assertLog(['Image requested [A]']); expect(getSuspenseyThingStatus('A')).toBe('pending'); - // We would expect this to be null if we did in fact suspend this commit - expect(root).toMatchRenderedOutput(); + // Suspend the initial mount + expect(root).toMatchRenderedOutput(null); resolveSuspenseyThing('A'); expect(getSuspenseyThingStatus('A')).toBe('fulfilled'); expect(root).toMatchRenderedOutput(); }); - // @TODO This isn't actually ideal behavior. We would really want the commit to suspend - // even if it is forced to be sync because we don't want to FOUC but refactoring the sync - // pathway is too risky to land right now so we just accept that we can still FOUC in this - // very specific case. - it('does not suspend commit during urgent update at the root when sync rendering', async () => { + it('does suspend commit during urgent update at the root when sync rendering', async () => { const root = ReactNoop.createRoot(); await act(() => resolveSuspenseyThing('A')); expect(getSuspenseyThingStatus('A')).toBe('fulfilled'); @@ -283,8 +275,8 @@ describe('ReactSuspenseyCommitPhase', () => { }); assertLog(['Image requested [B]']); expect(getSuspenseyThingStatus('B')).toBe('pending'); - // We would expect this to be hidden if we did in fact suspend this commit - expect(root).toMatchRenderedOutput(); + // Suspend and remain on previous screen + expect(root).toMatchRenderedOutput(); resolveSuspenseyThing('B'); expect(getSuspenseyThingStatus('B')).toBe('fulfilled');