Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fiber] Don't work on scheduled tasks while we're in an async commit but flush it eagerly if we're sync #31987

Merged
merged 12 commits into from
Jan 6, 2025
Merged
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHotReloading.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type {ReactNodeList} from 'shared/ReactTypes';
import {
flushSyncWork,
scheduleUpdateOnFiber,
flushPassiveEffects,
flushPendingEffects,
} from './ReactFiberWorkLoop';
import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';
import {updateContainerSync} from './ReactFiberReconciler';
Expand Down Expand Up @@ -229,7 +229,7 @@ export const scheduleRefresh: ScheduleRefresh = (
return;
}
const {staleFamilies, updatedFamilies} = update;
flushPassiveEffects();
flushPendingEffects();
scheduleFibersWithFamiliesRecursively(
root.current,
updatedFamilies,
Expand Down
9 changes: 5 additions & 4 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import isArray from 'shared/isArray';
import {
enableSchedulingProfiler,
enableHydrationLaneScheduling,
disableLegacyMode,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -75,7 +76,7 @@ import {
isAlreadyRendering,
deferredUpdates,
discreteUpdates,
flushPassiveEffects,
flushPendingEffects,
} from './ReactFiberWorkLoop';
import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';
import {
Expand Down Expand Up @@ -364,8 +365,8 @@ export function updateContainerSync(
parentComponent: ?React$Component<any, any>,
callback: ?Function,
): Lane {
if (container.tag === LegacyRoot) {
flushPassiveEffects();
if (!disableLegacyMode && container.tag === LegacyRoot) {
flushPendingEffects();
}
const current = container.current;
updateContainerImpl(
Expand Down Expand Up @@ -453,7 +454,7 @@ export {
flushSyncFromReconciler,
flushSyncWork,
isAlreadyRendering,
flushPassiveEffects,
flushPendingEffects as flushPassiveEffects,
};

export function getPublicRootInstance(
Expand Down
20 changes: 17 additions & 3 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@ import {
CommitContext,
NoContext,
RenderContext,
flushPassiveEffects,
flushPendingEffects,
getExecutionContext,
getWorkInProgressRoot,
getWorkInProgressRootRenderLanes,
getRootWithPendingPassiveEffects,
getPendingPassiveEffectsLanes,
hasPendingCommitEffects,
isWorkLoopSuspendedOnData,
performWorkOnRoot,
} from './ReactFiberWorkLoop';
Expand Down Expand Up @@ -466,10 +467,23 @@ function performWorkOnRootViaSchedulerTask(
trackSchedulerEvent();
}

if (hasPendingCommitEffects()) {
// We are currently in the middle of an async committing (such as a View Transition).
// We could force these to flush eagerly but it's better to defer any work until
// it finishes. This may not be the same root as we're waiting on.
// TODO: This relies on the commit eventually calling ensureRootIsScheduled which
// always calls processRootScheduleInMicrotask which in turn always loops through
// all the roots to figure out. This is all a bit inefficient and if optimized
// it'll need to consider rescheduling a task for any skipped roots.
root.callbackNode = null;
root.callbackPriority = NoLane;
return null;
}

// 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();
const didFlushPassiveEffects = flushPendingEffects(true);
if (didFlushPassiveEffects) {
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved
// Something in the passive effect phase may have canceled the current task.
// Check if the task node for this root was changed.
Expand Down Expand Up @@ -534,7 +548,7 @@ function performWorkOnRootViaSchedulerTask(
function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) {
// This is the entry point for synchronous tasks that don't go
// through Scheduler.
const didFlushPassiveEffects = flushPassiveEffects();
const didFlushPassiveEffects = flushPendingEffects();
if (didFlushPassiveEffects) {
// If passive effects were flushed, exit to the outer work loop in the root
// scheduler, so we can recompute the priority.
Expand Down
Loading
Loading