diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 57114af3e9..b2b50a6a52 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -51,6 +51,7 @@ import { requestCurrentTime, computeExpirationForFiber, scheduleWork, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, } from './ReactFiberScheduler'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; @@ -199,6 +200,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -218,6 +220,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, @@ -236,6 +239,7 @@ const classComponentUpdater = { update.callback = callback; } + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); enqueueUpdate(fiber, update); scheduleWork(fiber, expirationTime); }, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index bad7139e5b..d851bb77c2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -83,6 +83,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, requestCurrentTime, scheduleWork, } from './ReactFiberScheduler'; @@ -452,6 +453,7 @@ function commitLifeCycles( timedOutAt: NoWork, }; finishedWork.memoizedState = newState; + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(finishedWork); scheduleWork(finishedWork, Sync); return; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 439dffde48..7a2671c80c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -32,6 +32,7 @@ import { import { scheduleWork, computeExpirationForFiber, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, requestCurrentTime, } from './ReactFiberScheduler'; @@ -724,6 +725,7 @@ function dispatchAction( callback: callback !== undefined ? callback : null, next: null, }; + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber); // Append the update to the end of the list. const last = queue.last; if (last === null) { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index d594267a10..b06922c94f 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -52,6 +52,7 @@ import { syncUpdates, interactiveUpdates, flushInteractiveUpdates, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -145,9 +146,11 @@ function scheduleRootUpdate( ); update.callback = callback; } - enqueueUpdate(current, update); + flushPassiveEffectsBeforeSchedulingUpdateOnFiber(current); + enqueueUpdate(current, update); scheduleWork(current, expirationTime); + return expirationTime; } diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 307f7c8233..d257f3c93a 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -36,9 +36,6 @@ type BaseFiberRootProperties = {| // The currently active root fiber. This is the mutable root of the tree. current: Fiber, - serialEffectCallback: (() => mixed) | null, - serialEffectCallbackHandle: any, - // The following priority levels are used to distinguish between 1) // uncommitted work, 2) uncommitted work that is suspended, and 3) uncommitted // work that may be unsuspended. We choose not to track each individual @@ -117,8 +114,6 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, - serialEffectCallback: null, - serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, @@ -148,8 +143,6 @@ export function createFiberRoot( current: uninitializedFiber, containerInfo: containerInfo, pendingChildren: null, - serialEffectCallback: null, - serialEffectCallbackHandle: null, earliestPendingTime: NoWork, latestPendingTime: NoWork, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f7437c3726..a338c1d39d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -264,7 +264,9 @@ let nextRenderDidError: boolean = false; let nextEffect: Fiber | null = null; let isCommitting: boolean = false; -let needsPassiveCommit: boolean = false; +let rootWithPendingPassiveEffects: FiberRoot | null = null; +let firstPassiveEffect: Fiber | null = null; +let passiveEffectCallbackHandle: * = null; let legacyErrorBoundariesThatAlreadyFailed: Set | null = null; @@ -504,7 +506,7 @@ function commitAllLifeCycles( } if (effectTag & Passive) { - needsPassiveCommit = true; + rootWithPendingPassiveEffects = finishedRoot; } nextEffect = nextEffect.nextEffect; @@ -512,6 +514,9 @@ function commitAllLifeCycles( } function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void { + rootWithPendingPassiveEffects = null; + passiveEffectCallbackHandle = null; + // Set this to true to prevent re-entrancy const previousIsRendering = isRendering; isRendering = true; @@ -566,21 +571,42 @@ function markLegacyErrorBoundaryAsFailed(instance: mixed) { } } -function commitRoot(root: FiberRoot, finishedWork: Fiber): void { - const existingSerialEffectCallback = root.serialEffectCallback; - const existingSerialEffectCallbackHandle = root.serialEffectCallbackHandle; - if (existingSerialEffectCallback !== null) { - // A passive callback was scheduled during the previous commit, but it did - // not get a chance to flush. Flush it now to ensure serial execution. - // This should fire before any new mutations. - root.serialEffectCallback = null; - if (existingSerialEffectCallbackHandle !== null) { - root.serialEffectCallbackHandle = null; - Schedule_cancelCallback(existingSerialEffectCallbackHandle); - } - existingSerialEffectCallback(); +function flushPassiveEffectsBeforeSchedulingUpdateOnFiber(fiber: Fiber) { + if (rootWithPendingPassiveEffects !== null) { + // TODO: This is an unfortunate extra loop. We end up traversing to the root + // again in scheduleWorkToRoot. But we have to do this one first because it + // needs to happen before adding an update to the queue, and + // scheduleWorkToRoot may perform a synchronous re-render. Maybe we can + // solve this with batchedUpdates, or with the equivalent in the Scheduler + // package. + let node = fiber; + do { + switch (node.tag) { + case HostRoot: { + const root: FiberRoot = node.stateNode; + flushPassiveEffects(root); + return; + } + } + node = node.return; + } while (node !== null); } +} +function flushPassiveEffects(root: FiberRoot) { + if (root === rootWithPendingPassiveEffects) { + rootWithPendingPassiveEffects = null; + if (passiveEffectCallbackHandle !== null) { + Schedule_cancelCallback(passiveEffectCallbackHandle); + passiveEffectCallbackHandle = null; + } + if (firstPassiveEffect !== null) { + commitPassiveEffects(root, firstPassiveEffect); + } + } +} + +function commitRoot(root: FiberRoot, finishedWork: Fiber): void { isWorking = true; isCommitting = true; startCommitTimer(); @@ -770,31 +796,30 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void { } } - if (firstEffect !== null && needsPassiveCommit) { - const resolvedFirstEffect = firstEffect; + if (firstEffect !== null && rootWithPendingPassiveEffects !== null) { // This commit included a passive effect. These do not need to fire until // after the next paint. Schedule an callback to fire them in an async // event. To ensure serial execution, the callback will be flushed early if - // we enter another commit phase before then. - needsPassiveCommit = false; - let serialEffectCallback; + // we enter rootWithPendingPassiveEffects commit phase before then. + const resolvedFirstEffect = (firstPassiveEffect = firstEffect); + + let passiveEffectCallback; if (enableSchedulerTracing) { // TODO: Avoid this extra callback by mutating the tracing ref directly, // like we do at the beginning of commitRoot. I've opted not to do that // here because that code is still in flux. - serialEffectCallback = Schedule_tracing_wrap(() => { - root.serialEffectCallback = null; + passiveEffectCallback = Schedule_tracing_wrap(() => { commitPassiveEffects(root, resolvedFirstEffect); }); } else { - serialEffectCallback = () => { - root.serialEffectCallback = null; - commitPassiveEffects(root, resolvedFirstEffect); - }; + passiveEffectCallback = commitPassiveEffects.bind( + null, + root, + resolvedFirstEffect, + ); } - root.serialEffectCallback = serialEffectCallback; - root.serialEffectCallbackHandle = Schedule_scheduleCallback( - serialEffectCallback, + passiveEffectCallbackHandle = Schedule_scheduleCallback( + passiveEffectCallback, ); } @@ -1227,6 +1252,9 @@ function renderRoot( 'renderRoot was called recursively. This error is likely caused ' + 'by a bug in React. Please file an issue.', ); + + flushPassiveEffects(root); + isWorking = true; ReactCurrentOwner.currentDispatcher = Dispatcher; @@ -1505,11 +1533,8 @@ function renderRoot( onComplete(root, rootWorkInProgress, expirationTime); } -function dispatch( - sourceFiber: Fiber, - value: mixed, - expirationTime: ExpirationTime, -) { +function captureCommitPhaseError(sourceFiber: Fiber, value: mixed) { + const expirationTime = Sync; let fiber = sourceFiber.return; while (fiber !== null) { switch (fiber.tag) { @@ -1554,10 +1579,6 @@ function dispatch( } } -function captureCommitPhaseError(fiber: Fiber, error: mixed) { - return dispatch(fiber, error, Sync); -} - function computeThreadID( expirationTime: ExpirationTime, interactionThreadID: number, @@ -2587,4 +2608,5 @@ export { interactiveUpdates, flushInteractiveUpdates, computeUniqueAsyncExpiration, + flushPassiveEffectsBeforeSchedulingUpdateOnFiber, }; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 28577022d1..ec57236930 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -604,6 +604,38 @@ describe('ReactHooks', () => { expect(ReactNoop.clearYields()).toEqual(['Did commit [1]']); }); + it('flushes passive effects even with sibling deletions', () => { + function LayoutEffect(props) { + useLayoutEffect(() => { + ReactNoop.yield(`Layout effect`); + }); + return ; + } + function PassiveEffect(props) { + useEffect(() => { + ReactNoop.yield(`Passive effect`); + }, []); + return ; + } + let passive = ; + ReactNoop.render([, passive]); + expect(ReactNoop.flush()).toEqual(['Layout', 'Passive', 'Layout effect']); + expect(ReactNoop.getChildren()).toEqual([ + span('Layout'), + span('Passive'), + ]); + + // Destroying the first child shouldn't prevent the passive effect from + // being executed + ReactNoop.render([passive]); + expect(ReactNoop.flush()).toEqual(['Passive effect']); + expect(ReactNoop.getChildren()).toEqual([span('Passive')]); + + // (No effects are left to flush.) + flushPassiveEffects(); + expect(ReactNoop.clearYields()).toEqual(null); + }); + it( 'flushes effects serially by flushing old effects before flushing ' + "new ones, if they haven't already fired", @@ -631,9 +663,9 @@ describe('ReactHooks', () => { // Before the effects have a chance to flush, schedule another update ReactNoop.render(); expect(ReactNoop.flush()).toEqual([ - 1, - // The previous effect flushes before the host mutations + // The previous effect flushes before the reconciliation 'Committed state when effect was fired: 0', + 1, ]); expect(ReactNoop.getChildren()).toEqual([span(1)]); @@ -689,17 +721,39 @@ describe('ReactHooks', () => { // Rendering again should flush the previous commit's effects ReactNoop.render(); - ReactNoop.flushThrough([ - 'Count: (empty)', - 'Schedule update [0]', - 'Count: 0', - ]); + ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); + expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + flushPassiveEffects(); expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); + it('flushes serial effects before enqueueing work', () => { + let _updateCount; + function Counter(props) { + const [count, updateCount] = useState(0); + _updateCount = updateCount; + useEffect(() => { + ReactNoop.yield(`Will set count to 1`); + updateCount(1); + }, []); + return ; + } + ReactNoop.render(); + expect(ReactNoop.flush()).toEqual(['Count: 0']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); + + // Enqueuing this update forces the passive effect to be flushed -- + // updateCount(1) happens first, so 2 wins. + _updateCount(2); + expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); + expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); + }); + it( 'in sync mode, useEffect is deferred and updates finish synchronously ' + '(in a single batch)', @@ -1160,7 +1214,10 @@ describe('ReactHooks', () => { expect(committedText).toEqual('1'); flushPassiveEffects(); - expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + expect(ReactNoop.clearYields()).toEqual([ + 'Unmount normal [current: 1]', + 'Mount normal [current: 1]', + ]); }); it('force flushes passive effects before firing new layout effects', () => { @@ -1199,7 +1256,10 @@ describe('ReactHooks', () => { expect(committedText).toEqual('1'); flushPassiveEffects(); - expect(ReactNoop.clearYields()).toEqual(['Mount normal [current: 1]']); + expect(ReactNoop.clearYields()).toEqual([ + 'Unmount normal [current: 1]', + 'Mount normal [current: 1]', + ]); }); it('fires all mutation effects before firing any layout effects', () => {