@samitouri / QOS-React-2 / commits / 05dce7598a

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.

Andrew Clark committed Aug 1, 2019 at 22:17 UTC 05dce7598a60d38d39a6b32572b54e1408c29d9b

3 files changed +110 -23

packages/react-reconciler/src/ReactFiberCommitWork.js

+49 -19

@@ -22,6 +22,7 @@ import type {CapturedValue, CapturedError} from './ReactCapturedValue';

22 import type {SuspenseState} from './ReactFiberSuspenseComponent';

23 import type {FunctionComponentUpdateQueue} from './ReactFiberHooks';

24 import type {Thenable} from './ReactFiberWorkLoop';

25 +import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

27 import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';

28 import {

@@ -116,6 +117,7 @@ import {

117 MountPassive,

118 } from './ReactHookEffectTags';

119 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';

120 +import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';

121

122 let didWarnAboutUndefinedSnapshotBeforeUpdate: Set<mixed> | null = null;

123 if (__DEV__) {

@@ -716,7 +718,10 @@ function commitDetachRef(current: Fiber) {

718 // User-originating errors (lifecycles and refs) should not interrupt

719 // deletion, so don't let them throw. Host-originating errors should

720 // interrupt deletion, so it's okay

719 -function commitUnmount(current: Fiber): void {

721 +function commitUnmount(

722 + current: Fiber,

723 + renderPriorityLevel: ReactPriorityLevel,

724 +): void {

725 onCommitUnmount(current);

726

727 switch (current.tag) {

@@ -729,14 +734,33 @@ function commitUnmount(current: Fiber): void {

734 const lastEffect = updateQueue.lastEffect;

735 if (lastEffect !== null) {

736 const firstEffect = lastEffect.next;

732 - let effect = firstEffect;

733 - do {

734 - const destroy = effect.destroy;

735 - if (destroy !== undefined) {

736 - safelyCallDestroy(current, destroy);

737 - }

738 - effect = effect.next;

739 - } while (effect !== firstEffect);

737 +

738 + // When the owner fiber is deleted, the destroy function of a passive

739 + // effect hook is called during the synchronous commit phase. This is

740 + // a concession to implementation complexity. Calling it in the

741 + // passive effect phase (like they usually are, when dependencies

742 + // change during an update) would require either traversing the

743 + // children of the deleted fiber again, or including unmount effects

744 + // as part of the fiber effect list.

745 + //

746 + // Because this is during the sync commit phase, we need to change

747 + // the priority.

748 + //

749 + // TODO: Reconsider this implementation trade off.

750 + const priorityLevel =

751 + renderPriorityLevel > NormalPriority

752 + ? NormalPriority

753 + : renderPriorityLevel;

754 + runWithPriority(priorityLevel, () => {

755 + let effect = firstEffect;

756 + do {

757 + const destroy = effect.destroy;

758 + if (destroy !== undefined) {

759 + safelyCallDestroy(current, destroy);

760 + }

761 + effect = effect.next;

762 + } while (effect !== firstEffect);

763 + });

764 }

765 }

766 break;

@@ -777,7 +801,7 @@ function commitUnmount(current: Fiber): void {

801 // We are also not using this parent because

802 // the portal will get pushed immediately.

803 if (supportsMutation) {

780 - unmountHostComponents(current);

804 + unmountHostComponents(current, renderPriorityLevel);

805 } else if (supportsPersistence) {

806 emptyPortalContainer(current);

807 }

@@ -795,7 +819,10 @@ function commitUnmount(current: Fiber): void {

819 }

820 }

821

798 -function commitNestedUnmounts(root: Fiber): void {

822 +function commitNestedUnmounts(

823 + root: Fiber,

824 + renderPriorityLevel: ReactPriorityLevel,

825 +): void {

826 // While we're inside a removed host node we don't want to call

827 // removeChild on the inner nodes because they're removed by the top

828 // call anyway. We also want to call componentWillUnmount on all

@@ -803,7 +830,7 @@ function commitNestedUnmounts(root: Fiber): void {

830 // we do an inner loop while we're still inside the host node.

831 let node: Fiber = root;

832 while (true) {

806 - commitUnmount(node);

833 + commitUnmount(node, renderPriorityLevel);

834 // Visit children because they may contain more composite or host nodes.

835 // Skip portals because commitUnmount() currently visits them recursively.

836 if (

@@ -1054,7 +1081,7 @@ function commitPlacement(finishedWork: Fiber): void {

1081 }

1082 }

1083

1057 -function unmountHostComponents(current): void {

1084 +function unmountHostComponents(current, renderPriorityLevel): void {

1085 // We only have the top Fiber that was deleted but we need to recurse down its

1086 // children to find all the terminal nodes.

1087 let node: Fiber = current;

@@ -1102,7 +1129,7 @@ function unmountHostComponents(current): void {

1129 }

1130

1131 if (node.tag === HostComponent || node.tag === HostText) {

1105 - commitNestedUnmounts(node);

1132 + commitNestedUnmounts(node, renderPriorityLevel);

1133 // After all the children have unmounted, it is now safe to remove the

1134 // node from the tree.

1135 if (currentParentIsContainer) {

@@ -1119,7 +1146,7 @@ function unmountHostComponents(current): void {

1146 // Don't visit children because we already visited them.

1147 } else if (node.tag === FundamentalComponent) {

1148 const fundamentalNode = node.stateNode.instance;

1122 - commitNestedUnmounts(node);

1149 + commitNestedUnmounts(node, renderPriorityLevel);

1150 // After all the children have unmounted, it is now safe to remove the

1151 // node from the tree.

1152 if (currentParentIsContainer) {

@@ -1161,7 +1188,7 @@ function unmountHostComponents(current): void {

1188 continue;

1189 }

1190 } else {

1164 - commitUnmount(node);

1191 + commitUnmount(node, renderPriorityLevel);

1192 // Visit children because we may find more host components below.

1193 if (node.child !== null) {

1194 node.child.return = node;

@@ -1188,14 +1215,17 @@ function unmountHostComponents(current): void {

1215 }

1216 }

1217

1191 -function commitDeletion(current: Fiber): void {

1218 +function commitDeletion(

1219 + current: Fiber,

1220 + renderPriorityLevel: ReactPriorityLevel,

1221 +): void {

1222 if (supportsMutation) {

1223 // Recursively delete all host nodes from the parent.

1224 // Detach refs and call componentWillUnmount() on the whole subtree.

1195 - unmountHostComponents(current);

1225 + unmountHostComponents(current, renderPriorityLevel);

1226 } else {

1227 // Detach refs and call componentWillUnmount() on the whole subtree.

1198 - commitNestedUnmounts(current);

1228 + commitNestedUnmounts(current, renderPriorityLevel);

1229 }

1230 detachFiber(current);

1231 }

packages/react-reconciler/src/ReactFiberWorkLoop.js

+9 -4

@@ -1652,7 +1652,12 @@ function commitRootImpl(root, renderPriorityLevel) {

1652 nextEffect = firstEffect;

1653 do {

1654 if (__DEV__) {

1655 - invokeGuardedCallback(null, commitMutationEffects, null);

1655 + invokeGuardedCallback(

1656 + null,

1657 + commitMutationEffects,

1658 + null,

1659 + renderPriorityLevel,

1660 + );

1661 if (hasCaughtError()) {

1662 invariant(nextEffect !== null, 'Should be working on an effect.');

1663 const error = clearCaughtError();

@@ -1661,7 +1666,7 @@ function commitRootImpl(root, renderPriorityLevel) {

1666 }

1667 } else {

1668 try {

1664 - commitMutationEffects();

1669 + commitMutationEffects(renderPriorityLevel);

1670 } catch (error) {

1671 invariant(nextEffect !== null, 'Should be working on an effect.');

1672 captureCommitPhaseError(nextEffect, error);

@@ -1850,7 +1855,7 @@ function commitBeforeMutationEffects() {

1855 }

1856 }

1857

1853 -function commitMutationEffects() {

1858 +function commitMutationEffects(renderPriorityLevel) {

1859 // TODO: Should probably move the bulk of this function to commitWork.

1860 while (nextEffect !== null) {

1861 setCurrentDebugFiberInDEV(nextEffect);

@@ -1901,7 +1906,7 @@ function commitMutationEffects() {

1906 break;

1907 }

1908 case Deletion: {

1904 - commitDeletion(nextEffect);

1909 + commitDeletion(nextEffect, renderPriorityLevel);

1910 break;

1911 }

1912 }

packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js

+52

@@ -149,6 +149,11 @@ describe('ReactSchedulerIntegration', () => {

149 Scheduler.unstable_yieldValue(

150 `Effect priority: ${getCurrentPriorityAsString()}`,

151 );

152 + return () => {

153 + Scheduler.unstable_yieldValue(

154 + `Effect clean-up priority: ${getCurrentPriorityAsString()}`,

155 + );

156 + };

157 });

158 return null;

159 }

@@ -170,6 +175,7 @@ describe('ReactSchedulerIntegration', () => {

175 });

176 expect(Scheduler).toHaveYielded([

177 'Render priority: UserBlocking',

178 + 'Effect clean-up priority: Normal',

179 'Effect priority: Normal',

180 ]);

181

@@ -181,6 +187,7 @@ describe('ReactSchedulerIntegration', () => {

187 });

188 expect(Scheduler).toHaveYielded([

189 'Render priority: Idle',

190 + 'Effect clean-up priority: Idle',

191 'Effect priority: Idle',

192 ]);

193 });

@@ -213,6 +220,51 @@ describe('ReactSchedulerIntegration', () => {

220 ]);

221 });

222

223 + it('passive effect clean-up functions have correct priority even when component is deleted', async () => {

224 + const {useEffect} = React;

225 + function ReadPriority({step}) {

226 + useEffect(() => {

227 + return () => {

228 + Scheduler.unstable_yieldValue(

229 + `Effect clean-up priority: ${getCurrentPriorityAsString()}`,

230 + );

231 + };

232 + });

233 + return null;

234 + }

235 +

236 + await ReactNoop.act(async () => {

237 + ReactNoop.render(<ReadPriority />);

238 + });

239 + await ReactNoop.act(async () => {

240 + Scheduler.unstable_runWithPriority(ImmediatePriority, () => {

241 + ReactNoop.render(null);

242 + });

243 + });

244 + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Normal']);

245 +

246 + await ReactNoop.act(async () => {

247 + ReactNoop.render(<ReadPriority />);

248 + });

249 + await ReactNoop.act(async () => {

250 + Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {

251 + ReactNoop.render(null);

252 + });

253 + });

254 + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Normal']);

255 +

256 + // Renders lower than normal priority spawn effects at the same priority

257 + await ReactNoop.act(async () => {

258 + ReactNoop.render(<ReadPriority />);

259 + });

260 + await ReactNoop.act(async () => {

261 + Scheduler.unstable_runWithPriority(IdlePriority, () => {

262 + ReactNoop.render(null);

263 + });

264 + });

265 + expect(Scheduler).toHaveYielded(['Effect clean-up priority: Idle']);

266 + });

267 +

268 it('after completing a level of work, infers priority of the next batch based on its expiration time', () => {

269 function App({label}) {

270 Scheduler.unstable_yieldValue(