Bugfix: Don't unmount siblings of deleted node (#19516)
* Test: Don't unmount siblings of deleted node Adds a failing regression test. Will fix in the next commit. * Refactor to accept deleted fiber, not child list A deleted fiber is passed to flushPassiveUnmountEffectsInsideOfDeletedTree, but the code is written as if it accepts the first node of a child list. This is likely because the function was based on similar functions like `flushPassiveUnmountEffects`, which do accept a child list. Unfortunately, types don't help here because we use the first node in the list to represent the whole list, so in both cases, the type is Fiber. Might be worth changing the other functions to also accept individual fibers instead of a child list, to help avoid confusion. * Add layout effect to regression test, just in case
Andrew Clark committed
Aug 3, 2020 at 10:57 UTC
5f1890f12b4ea900a82853c528c0fe040760857c
2 files changed
+71
-33
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+19
-33
@@ -2790,16 +2790,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
2790
if (deletions !== null) {
2791
for (let i = 0; i < deletions.length; i++) {
2792
const fiberToDelete = deletions[i];
2793
- // If this fiber (or anything below it) has passive effects then traverse the subtree.
2794
- const primaryEffectTag = fiberToDelete.effectTag & PassiveStatic;
2795
- const primarySubtreeTag =
2796
- fiberToDelete.subtreeTag & PassiveStaticSubtreeTag;
2797
- if (
2798
- primarySubtreeTag !== NoSubtreeTag ||
2799
- primaryEffectTag !== NoEffect
2800
- ) {
2801
- flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
2802
- }
2793
+ flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
2794
2795
// Now that passive effects have been processed, it's safe to detach lingering pointers.
2796
detachFiberAfterEffects(fiberToDelete);
@@ -2835,34 +2826,29 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
2826
}
2827
2828
function flushPassiveUnmountEffectsInsideOfDeletedTree(
2838
- firstChild: Fiber,
2829
+ fiberToDelete: Fiber,
2830
): void {
2840
- let fiber = firstChild;
2841
- while (fiber !== null) {
2842
- const child = fiber.child;
2843
- if (child !== null) {
2844
- // If any children have passive effects then traverse the subtree.
2845
- // Note that this requires checking subtreeTag of the current Fiber,
2846
- // rather than the subtreeTag/effectsTag of the first child,
2847
- // since that would not cover passive effects in siblings.
2848
- const primarySubtreeTag = fiber.subtreeTag & PassiveStaticSubtreeTag;
2849
- if (primarySubtreeTag !== NoSubtreeTag) {
2850
- flushPassiveUnmountEffectsInsideOfDeletedTree(child);
2851
- }
2831
+ if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
2832
+ // If any children have passive effects then traverse the subtree.
2833
+ // Note that this requires checking subtreeTag of the current Fiber,
2834
+ // rather than the subtreeTag/effectsTag of the first child,
2835
+ // since that would not cover passive effects in siblings.
2836
+ let child = fiberToDelete.child;
2837
+ while (child !== null) {
2838
+ flushPassiveUnmountEffectsInsideOfDeletedTree(child);
2839
+ child = child.sibling;
2840
}
2841
+ }
2842
2854
- if ((fiber.effectTag & PassiveStatic) !== NoEffect) {
2855
- switch (fiber.tag) {
2856
- case FunctionComponent:
2857
- case ForwardRef:
2858
- case SimpleMemoComponent:
2859
- case Block: {
2860
- flushPassiveUnmountEffectsImpl(fiber, HookPassive);
2861
- }
2843
+ if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
2844
+ switch (fiberToDelete.tag) {
2845
+ case FunctionComponent:
2846
+ case ForwardRef:
2847
+ case SimpleMemoComponent:
2848
+ case Block: {
2849
+ flushPassiveUnmountEffectsImpl(fiberToDelete, HookPassive);
2850
}
2851
}
2864
-
2865
- fiber = fiber.sibling;
2852
}
2853
}
2854
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+52
@@ -3314,4 +3314,56 @@ describe('ReactHooksWithNoopRenderer', () => {
3314
});
3315
expect(ReactNoop).toMatchRenderedOutput('ABC');
3316
});
3317
+
3318
+ it("regression test: don't unmount effects on siblings of deleted nodes", async () => {
3319
+ const root = ReactNoop.createRoot();
3320
+
3321
+ function Child({label}) {
3322
+ useLayoutEffect(() => {
3323
+ Scheduler.unstable_yieldValue('Mount layout ' + label);
3324
+ return () => {
3325
+ Scheduler.unstable_yieldValue('Unmount layout ' + label);
3326
+ };
3327
+ }, [label]);
3328
+ useEffect(() => {
3329
+ Scheduler.unstable_yieldValue('Mount passive ' + label);
3330
+ return () => {
3331
+ Scheduler.unstable_yieldValue('Unmount passive ' + label);
3332
+ };
3333
+ }, [label]);
3334
+ return label;
3335
+ }
3336
+
3337
+ await act(async () => {
3338
+ root.render(
3339
+ <>
3340
+ <Child key="A" label="A" />
3341
+ <Child key="B" label="B" />
3342
+ </>,
3343
+ );
3344
+ });
3345
+ expect(Scheduler).toHaveYielded([
3346
+ 'Mount layout A',
3347
+ 'Mount layout B',
3348
+ 'Mount passive A',
3349
+ 'Mount passive B',
3350
+ ]);
3351
+
3352
+ // Delete A. This should only unmount the effect on A. In the regression,
3353
+ // B's effect would also unmount.
3354
+ await act(async () => {
3355
+ root.render(
3356
+ <>
3357
+ <Child key="B" label="B" />
3358
+ </>,
3359
+ );
3360
+ });
3361
+ expect(Scheduler).toHaveYielded(['Unmount layout A', 'Unmount passive A']);
3362
+
3363
+ // Now delete and unmount B.
3364
+ await act(async () => {
3365
+ root.render(null);
3366
+ });
3367
+ expect(Scheduler).toHaveYielded(['Unmount layout B', 'Unmount passive B']);
3368
+ });
3369
});