From 6514697f0cb294b48550eeb3bcbddfd11143539c Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Wed, 3 Oct 2018 18:31:35 -0700 Subject: [PATCH] Make sure deletions don't stop passive effects Before the fix, the passive effect in the test is never executed. We were previously waiting until the next commit phase to run effects. Now, we run them before scheduling work. --- .../src/ReactFiberClassComponent.js | 4 + .../src/ReactFiberCommitWork.js | 2 + .../react-reconciler/src/ReactFiberHooks.js | 2 + .../src/ReactFiberReconciler.js | 5 +- .../react-reconciler/src/ReactFiberRoot.js | 7 -- .../src/ReactFiberScheduler.js | 98 ++++++++++++------- .../src/__tests__/ReactHooks-test.internal.js | 78 +++++++++++++-- 7 files changed, 141 insertions(+), 55 deletions(-) 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', () => {