From 05dce7598a60d38d39a6b32572b54e1408c29d9b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 1 Aug 2019 22:17:59 -0700 Subject: [PATCH] Fix priority of clean-up function on deletion (#16277) The clean-up function of a passive effect (`useEffect`) usually fires in a post-commit task, after the browser has painted. However, there is an exception when the component (or its parent) is deleted from the tree. In that case, we fire the clean-up function during the synchronous commit phase, the same phase we use for layout effects. This is a concession to implementation complexity. Calling it in the passive effect phase would require either traversing the children of the deleted fiber again, or including unmount effects as part of the fiber effect list. Because the functions are called during the sync phase in this case, the Scheduler priority is Immediate (the one used for layout) instead of Normal. We may want to reconsider this trade off later. --- .../src/ReactFiberCommitWork.js | 68 +++++++++++++------ .../src/ReactFiberWorkLoop.js | 13 ++-- ...ReactSchedulerIntegration-test.internal.js | 52 ++++++++++++++ 3 files changed, 110 insertions(+), 23 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 8f3156041c0fa..ee20c789b94fd 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -22,6 +22,7 @@ import type {CapturedValue, CapturedError} from './ReactCapturedValue'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks'; import type {Thenable} from './ReactFiberWorkLoop'; +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import { @@ -116,6 +117,7 @@ import { MountPassive, } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork'; +import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration'; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -716,7 +718,10 @@ function commitDetachRef(current: Fiber) { // User-originating errors (lifecycles and refs) should not interrupt // deletion, so don't let them throw. Host-originating errors should // interrupt deletion, so it's okay -function commitUnmount(current: Fiber): void { +function commitUnmount( + current: Fiber, + renderPriorityLevel: ReactPriorityLevel, +): void { onCommitUnmount(current); switch (current.tag) { @@ -729,14 +734,33 @@ function commitUnmount(current: Fiber): void { const lastEffect = updateQueue.lastEffect; if (lastEffect !== null) { const firstEffect = lastEffect.next; - let effect = firstEffect; - do { - const destroy = effect.destroy; - if (destroy !== undefined) { - safelyCallDestroy(current, destroy); - } - effect = effect.next; - } while (effect !== firstEffect); + + // When the owner fiber is deleted, the destroy function of a passive + // effect hook is called during the synchronous commit phase. This is + // a concession to implementation complexity. Calling it in the + // passive effect phase (like they usually are, when dependencies + // change during an update) would require either traversing the + // children of the deleted fiber again, or including unmount effects + // as part of the fiber effect list. + // + // Because this is during the sync commit phase, we need to change + // the priority. + // + // TODO: Reconsider this implementation trade off. + const priorityLevel = + renderPriorityLevel > NormalPriority + ? NormalPriority + : renderPriorityLevel; + runWithPriority(priorityLevel, () => { + let effect = firstEffect; + do { + const destroy = effect.destroy; + if (destroy !== undefined) { + safelyCallDestroy(current, destroy); + } + effect = effect.next; + } while (effect !== firstEffect); + }); } } break; @@ -777,7 +801,7 @@ function commitUnmount(current: Fiber): void { // We are also not using this parent because // the portal will get pushed immediately. if (supportsMutation) { - unmountHostComponents(current); + unmountHostComponents(current, renderPriorityLevel); } else if (supportsPersistence) { emptyPortalContainer(current); } @@ -795,7 +819,10 @@ function commitUnmount(current: Fiber): void { } } -function commitNestedUnmounts(root: Fiber): void { +function commitNestedUnmounts( + root: Fiber, + renderPriorityLevel: ReactPriorityLevel, +): void { // While we're inside a removed host node we don't want to call // removeChild on the inner nodes because they're removed by the top // call anyway. We also want to call componentWillUnmount on all @@ -803,7 +830,7 @@ function commitNestedUnmounts(root: Fiber): void { // we do an inner loop while we're still inside the host node. let node: Fiber = root; while (true) { - commitUnmount(node); + commitUnmount(node, renderPriorityLevel); // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if ( @@ -1054,7 +1081,7 @@ function commitPlacement(finishedWork: Fiber): void { } } -function unmountHostComponents(current): void { +function unmountHostComponents(current, renderPriorityLevel): void { // We only have the top Fiber that was deleted but we need to recurse down its // children to find all the terminal nodes. let node: Fiber = current; @@ -1102,7 +1129,7 @@ function unmountHostComponents(current): void { } if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(node); + commitNestedUnmounts(node, renderPriorityLevel); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1119,7 +1146,7 @@ function unmountHostComponents(current): void { // Don't visit children because we already visited them. } else if (node.tag === FundamentalComponent) { const fundamentalNode = node.stateNode.instance; - commitNestedUnmounts(node); + commitNestedUnmounts(node, renderPriorityLevel); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1161,7 +1188,7 @@ function unmountHostComponents(current): void { continue; } } else { - commitUnmount(node); + commitUnmount(node, renderPriorityLevel); // Visit children because we may find more host components below. if (node.child !== null) { node.child.return = node; @@ -1188,14 +1215,17 @@ function unmountHostComponents(current): void { } } -function commitDeletion(current: Fiber): void { +function commitDeletion( + current: Fiber, + renderPriorityLevel: ReactPriorityLevel, +): void { if (supportsMutation) { // Recursively delete all host nodes from the parent. // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(current); + unmountHostComponents(current, renderPriorityLevel); } else { // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(current); + commitNestedUnmounts(current, renderPriorityLevel); } detachFiber(current); } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 375f7a5e94071..76698b67185b2 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1652,7 +1652,12 @@ function commitRootImpl(root, renderPriorityLevel) { nextEffect = firstEffect; do { if (__DEV__) { - invokeGuardedCallback(null, commitMutationEffects, null); + invokeGuardedCallback( + null, + commitMutationEffects, + null, + renderPriorityLevel, + ); if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); @@ -1661,7 +1666,7 @@ function commitRootImpl(root, renderPriorityLevel) { } } else { try { - commitMutationEffects(); + commitMutationEffects(renderPriorityLevel); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); captureCommitPhaseError(nextEffect, error); @@ -1850,7 +1855,7 @@ function commitBeforeMutationEffects() { } } -function commitMutationEffects() { +function commitMutationEffects(renderPriorityLevel) { // TODO: Should probably move the bulk of this function to commitWork. while (nextEffect !== null) { setCurrentDebugFiberInDEV(nextEffect); @@ -1901,7 +1906,7 @@ function commitMutationEffects() { break; } case Deletion: { - commitDeletion(nextEffect); + commitDeletion(nextEffect, renderPriorityLevel); break; } } diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js index 95f4239fde883..eca152701205c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js @@ -149,6 +149,11 @@ describe('ReactSchedulerIntegration', () => { Scheduler.unstable_yieldValue( `Effect priority: ${getCurrentPriorityAsString()}`, ); + return () => { + Scheduler.unstable_yieldValue( + `Effect clean-up priority: ${getCurrentPriorityAsString()}`, + ); + }; }); return null; } @@ -170,6 +175,7 @@ describe('ReactSchedulerIntegration', () => { }); expect(Scheduler).toHaveYielded([ 'Render priority: UserBlocking', + 'Effect clean-up priority: Normal', 'Effect priority: Normal', ]); @@ -181,6 +187,7 @@ describe('ReactSchedulerIntegration', () => { }); expect(Scheduler).toHaveYielded([ 'Render priority: Idle', + 'Effect clean-up priority: Idle', 'Effect priority: Idle', ]); }); @@ -213,6 +220,51 @@ describe('ReactSchedulerIntegration', () => { ]); }); + it('passive effect clean-up functions have correct priority even when component is deleted', async () => { + const {useEffect} = React; + function ReadPriority({step}) { + useEffect(() => { + return () => { + Scheduler.unstable_yieldValue( + `Effect clean-up priority: ${getCurrentPriorityAsString()}`, + ); + }; + }); + return null; + } + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + await ReactNoop.act(async () => { + Scheduler.unstable_runWithPriority(ImmediatePriority, () => { + ReactNoop.render(null); + }); + }); + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Normal']); + + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + await ReactNoop.act(async () => { + Scheduler.unstable_runWithPriority(UserBlockingPriority, () => { + ReactNoop.render(null); + }); + }); + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Normal']); + + // Renders lower than normal priority spawn effects at the same priority + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + await ReactNoop.act(async () => { + Scheduler.unstable_runWithPriority(IdlePriority, () => { + ReactNoop.render(null); + }); + }); + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Idle']); + }); + it('after completing a level of work, infers priority of the next batch based on its expiration time', () => { function App({label}) { Scheduler.unstable_yieldValue(