Consolidate commit phase hook functions (#19864)
There were a few pairs of commit phase functions that were almost identical except for one detail. I've refactored them a bit to consolidate their implementations: - Lifted error handling logic when mounting a fiber's passive hook effects to surround the entire list, instead of surrounding each effect. - Lifted profiler duration tracking to surround the entire list. In both cases, this matches the corresponding code for the layout phase. The naming is still a bit of a mess but I'm not too concerned because my next step is to refactor each commit sub-phase (layout, mutation) so that we can store values on the JS stack. So the existing function boundaries are about to change, anyway.
Andrew Clark committed
Sep 22, 2020 at 13:16 UTC
7355bf575ad0e5fa84061287df8d6330d0a4803f
2 files changed
+86
-141
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+60
-133
@@ -20,10 +20,7 @@ import type {FiberRoot} from './ReactInternalTypes';
20
import type {Lanes} from './ReactFiberLane';
21
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
22
import type {UpdateQueue} from './ReactUpdateQueue.new';
23
-import type {
24
- Effect as HookEffect,
25
- FunctionComponentUpdateQueue,
26
-} from './ReactFiberHooks.new';
23
+import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
24
import type {Wakeable} from 'shared/ReactTypes';
25
import type {ReactPriorityLevel} from './ReactInternalTypes';
26
import type {OffscreenState} from './ReactFiberOffscreenComponent';
@@ -343,42 +340,6 @@ function commitHookEffectListUnmount(
340
}
341
}
342
346
-// TODO: Remove this duplication.
347
-function commitHookEffectListUnmount2(
348
- // Tags to check for when deciding whether to unmount. e.g. to skip over layout effects
349
- hookFlags: HookFlags,
350
- fiber: Fiber,
351
- nearestMountedAncestor: Fiber | null,
352
-): void {
353
- const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
354
- const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
355
- if (lastEffect !== null) {
356
- const firstEffect = lastEffect.next;
357
- let effect = firstEffect;
358
- do {
359
- const {next, tag} = effect;
360
- if ((tag & hookFlags) === hookFlags) {
361
- const destroy = effect.destroy;
362
- if (destroy !== undefined) {
363
- effect.destroy = undefined;
364
- if (
365
- enableProfilerTimer &&
366
- enableProfilerCommitHooks &&
367
- fiber.mode & ProfileMode
368
- ) {
369
- startPassiveEffectTimer();
370
- safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
371
- recordPassiveEffectDuration(fiber);
372
- } else {
373
- safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
374
- }
375
- }
376
- }
377
- effect = next;
378
- } while (effect !== firstEffect);
379
- }
380
-}
381
-
343
function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) {
344
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
345
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
@@ -429,83 +390,6 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) {
390
}
391
}
392
432
-function invokePassiveEffectCreate(effect: HookEffect): void {
433
- const create = effect.create;
434
- effect.destroy = create();
435
-}
436
-
437
-// TODO: Remove this duplication.
438
-function commitHookEffectListMount2(fiber: Fiber): void {
439
- const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
440
- const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
441
- if (lastEffect !== null) {
442
- const firstEffect = lastEffect.next;
443
- let effect = firstEffect;
444
- do {
445
- const {next, tag} = effect;
446
-
447
- if (
448
- (tag & HookPassive) !== NoHookEffect &&
449
- (tag & HookHasEffect) !== NoHookEffect
450
- ) {
451
- if (__DEV__) {
452
- if (
453
- enableProfilerTimer &&
454
- enableProfilerCommitHooks &&
455
- fiber.mode & ProfileMode
456
- ) {
457
- startPassiveEffectTimer();
458
- invokeGuardedCallback(
459
- null,
460
- invokePassiveEffectCreate,
461
- null,
462
- effect,
463
- );
464
- recordPassiveEffectDuration(fiber);
465
- } else {
466
- invokeGuardedCallback(
467
- null,
468
- invokePassiveEffectCreate,
469
- null,
470
- effect,
471
- );
472
- }
473
- if (hasCaughtError()) {
474
- invariant(fiber !== null, 'Should be working on an effect.');
475
- const error = clearCaughtError();
476
- captureCommitPhaseError(fiber, fiber.return, error);
477
- }
478
- } else {
479
- try {
480
- const create = effect.create;
481
- if (
482
- enableProfilerTimer &&
483
- enableProfilerCommitHooks &&
484
- fiber.mode & ProfileMode
485
- ) {
486
- try {
487
- startPassiveEffectTimer();
488
- effect.destroy = create();
489
- } finally {
490
- recordPassiveEffectDuration(fiber);
491
- }
492
- } else {
493
- effect.destroy = create();
494
- }
495
- // TODO: This is missing the warning that exists in commitHookEffectListMount.
496
- // The warning refers to useEffect but only applies to useLayoutEffect.
497
- } catch (error) {
498
- invariant(fiber !== null, 'Should be working on an effect.');
499
- captureCommitPhaseError(fiber, fiber.return, error);
500
- }
501
- }
502
- }
503
-
504
- effect = next;
505
- } while (effect !== firstEffect);
506
- }
507
-}
508
-
393
function commitProfilerPassiveEffect(
394
finishedRoot: FiberRoot,
395
finishedWork: Fiber,
@@ -1894,17 +1778,31 @@ function commitResetTextContent(current: Fiber): void {
1778
resetTextContent(current.stateNode);
1779
}
1780
1897
-function commitPassiveWork(finishedWork: Fiber): void {
1781
+function commitPassiveUnmountInsideDeletedTree(finishedWork: Fiber): void {
1782
switch (finishedWork.tag) {
1783
case FunctionComponent:
1784
case ForwardRef:
1785
case SimpleMemoComponent:
1786
case Block: {
1903
- commitHookEffectListUnmount2(
1904
- HookPassive | HookHasEffect,
1905
- finishedWork,
1906
- finishedWork.return,
1907
- );
1787
+ if (
1788
+ enableProfilerTimer &&
1789
+ enableProfilerCommitHooks &&
1790
+ finishedWork.mode & ProfileMode
1791
+ ) {
1792
+ startPassiveEffectTimer();
1793
+ commitHookEffectListUnmount(
1794
+ HookPassive | HookHasEffect,
1795
+ finishedWork,
1796
+ finishedWork.return,
1797
+ );
1798
+ recordPassiveEffectDuration(finishedWork);
1799
+ } else {
1800
+ commitHookEffectListUnmount(
1801
+ HookPassive | HookHasEffect,
1802
+ finishedWork,
1803
+ finishedWork.return,
1804
+ );
1805
+ }
1806
break;
1807
}
1808
}
@@ -1918,16 +1816,32 @@ function commitPassiveUnmount(
1816
case FunctionComponent:
1817
case ForwardRef:
1818
case SimpleMemoComponent:
1921
- case Block:
1922
- commitHookEffectListUnmount2(
1923
- HookPassive,
1924
- current,
1925
- nearestMountedAncestor,
1926
- );
1819
+ case Block: {
1820
+ if (
1821
+ enableProfilerTimer &&
1822
+ enableProfilerCommitHooks &&
1823
+ current.mode & ProfileMode
1824
+ ) {
1825
+ startPassiveEffectTimer();
1826
+ commitHookEffectListUnmount(
1827
+ HookPassive,
1828
+ current,
1829
+ nearestMountedAncestor,
1830
+ );
1831
+ recordPassiveEffectDuration(current);
1832
+ } else {
1833
+ commitHookEffectListUnmount(
1834
+ HookPassive,
1835
+ current,
1836
+ nearestMountedAncestor,
1837
+ );
1838
+ }
1839
+ break;
1840
+ }
1841
}
1842
}
1843
1930
-function commitPassiveLifeCycles(
1844
+function commitPassiveMount(
1845
finishedRoot: FiberRoot,
1846
finishedWork: Fiber,
1847
): void {
@@ -1936,7 +1850,20 @@ function commitPassiveLifeCycles(
1850
case ForwardRef:
1851
case SimpleMemoComponent:
1852
case Block: {
1939
- commitHookEffectListMount2(finishedWork);
1853
+ if (
1854
+ enableProfilerTimer &&
1855
+ enableProfilerCommitHooks &&
1856
+ finishedWork.mode & ProfileMode
1857
+ ) {
1858
+ startPassiveEffectTimer();
1859
+ try {
1860
+ commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
1861
+ } finally {
1862
+ recordPassiveEffectDuration(finishedWork);
1863
+ }
1864
+ } else {
1865
+ commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
1866
+ }
1867
break;
1868
}
1869
case Profiler: {
@@ -1956,6 +1883,6 @@ export {
1883
commitAttachRef,
1884
commitDetachRef,
1885
commitPassiveUnmount,
1959
- commitPassiveWork,
1960
- commitPassiveLifeCycles,
1886
+ commitPassiveUnmountInsideDeletedTree,
1887
+ commitPassiveMount,
1888
};
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+26
-8
@@ -191,9 +191,9 @@ import {
191
commitPlacement,
192
commitWork,
193
commitDeletion,
194
- commitPassiveUnmount,
195
- commitPassiveWork,
196
- commitPassiveLifeCycles as commitPassiveEffectOnFiber,
194
+ commitPassiveUnmount as commitPassiveUnmountOnFiber,
195
+ commitPassiveUnmountInsideDeletedTree as commitPassiveUnmountInsideDeletedTreeOnFiber,
196
+ commitPassiveMount as commitPassiveMountOnFiber,
197
commitDetachRef,
198
commitAttachRef,
199
commitResetTextContent,
@@ -2458,9 +2458,27 @@ function flushPassiveMountEffects(root, firstChild: Fiber): void {
2458
}
2459
2460
if ((fiber.flags & Passive) !== NoFlags) {
2461
- setCurrentDebugFiberInDEV(fiber);
2462
- commitPassiveEffectOnFiber(root, fiber);
2463
- resetCurrentDebugFiberInDEV();
2461
+ if (__DEV__) {
2462
+ setCurrentDebugFiberInDEV(fiber);
2463
+ invokeGuardedCallback(
2464
+ null,
2465
+ commitPassiveMountOnFiber,
2466
+ null,
2467
+ root,
2468
+ fiber,
2469
+ );
2470
+ if (hasCaughtError()) {
2471
+ const error = clearCaughtError();
2472
+ captureCommitPhaseError(fiber, fiber.return, error);
2473
+ }
2474
+ resetCurrentDebugFiberInDEV();
2475
+ } else {
2476
+ try {
2477
+ commitPassiveMountOnFiber(root, fiber);
2478
+ } catch (error) {
2479
+ captureCommitPhaseError(fiber, fiber.return, error);
2480
+ }
2481
+ }
2482
}
2483
2484
fiber = fiber.sibling;
@@ -2496,7 +2514,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
2514
const primaryFlags = fiber.flags & Passive;
2515
if (primaryFlags !== NoFlags) {
2516
setCurrentDebugFiberInDEV(fiber);
2499
- commitPassiveWork(fiber);
2517
+ commitPassiveUnmountInsideDeletedTreeOnFiber(fiber);
2518
resetCurrentDebugFiberInDEV();
2519
}
2520
@@ -2525,7 +2543,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
2543
2544
if ((fiberToDelete.flags & PassiveStatic) !== NoFlags) {
2545
setCurrentDebugFiberInDEV(fiberToDelete);
2528
- commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
2546
+ commitPassiveUnmountOnFiber(fiberToDelete, nearestMountedAncestor);
2547
resetCurrentDebugFiberInDEV();
2548
}
2549
}