Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted (#24400)
* Bug: Missing unmount when suspended tree deleted When a suspended tree switches to a fallback, we unmount the effects. If the suspended tree is then deleted, there's a guard to prevent us from unmounting the effects again. However, in legacy mode, we don't unmount effects when a tree suspends. So if the suspended tree is then deleted, we do need to unmount the effects. We're missing a check for legacy/concurrent mode. * Fix: Unmount suspended tree when it is deleted
Andrew Clark committed
Apr 19, 2022 at 17:09 UTC
ab9cdd34fb6c88d56c83882dbd011b4546878483
3 files changed
+139
-104
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+45
-25
@@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber(
1804
return;
1805
}
1806
case OffscreenComponent: {
1807
- // If this offscreen component is hidden, we already unmounted it. Before
1808
- // deleting the children, track that it's already unmounted so that we
1809
- // don't attempt to unmount the effects again.
1810
- // TODO: If the tree is hidden, in most cases we should be able to skip
1811
- // over the nested children entirely. An exception is we haven't yet found
1812
- // the topmost host node to delete, which we already track on the stack.
1813
- // But the other case is portals, which need to be detached no matter how
1814
- // deeply they are nested. We should use a subtree flag to track whether a
1815
- // subtree includes a nested portal.
1816
- const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1817
- offscreenSubtreeWasHidden =
1818
- prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1819
- recursivelyTraverseDeletionEffects(
1820
- finishedRoot,
1821
- nearestMountedAncestor,
1822
- deletedFiber,
1823
- );
1824
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1807
+ if (
1808
+ // TODO: Remove this dead flag
1809
+ enableSuspenseLayoutEffectSemantics &&
1810
+ deletedFiber.mode & ConcurrentMode
1811
+ ) {
1812
+ // If this offscreen component is hidden, we already unmounted it. Before
1813
+ // deleting the children, track that it's already unmounted so that we
1814
+ // don't attempt to unmount the effects again.
1815
+ // TODO: If the tree is hidden, in most cases we should be able to skip
1816
+ // over the nested children entirely. An exception is we haven't yet found
1817
+ // the topmost host node to delete, which we already track on the stack.
1818
+ // But the other case is portals, which need to be detached no matter how
1819
+ // deeply they are nested. We should use a subtree flag to track whether a
1820
+ // subtree includes a nested portal.
1821
+ const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1822
+ offscreenSubtreeWasHidden =
1823
+ prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1824
+ recursivelyTraverseDeletionEffects(
1825
+ finishedRoot,
1826
+ nearestMountedAncestor,
1827
+ deletedFiber,
1828
+ );
1829
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1830
+ } else {
1831
+ recursivelyTraverseDeletionEffects(
1832
+ finishedRoot,
1833
+ nearestMountedAncestor,
1834
+ deletedFiber,
1835
+ );
1836
+ }
1837
break;
1838
}
1839
default: {
@@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber(
2250
case OffscreenComponent: {
2251
const wasHidden = current !== null && current.memoizedState !== null;
2252
2241
- // Before committing the children, track on the stack whether this
2242
- // offscreen subtree was already hidden, so that we don't unmount the
2243
- // effects again.
2244
- const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2245
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
2246
- recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2247
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2253
+ if (
2254
+ // TODO: Remove this dead flag
2255
+ enableSuspenseLayoutEffectSemantics &&
2256
+ finishedWork.mode & ConcurrentMode
2257
+ ) {
2258
+ // Before committing the children, track on the stack whether this
2259
+ // offscreen subtree was already hidden, so that we don't unmount the
2260
+ // effects again.
2261
+ const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2262
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
2263
+ recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2264
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2265
+ } else {
2266
+ recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2267
+ }
2268
2269
commitReconciliationEffects(finishedWork);
2270
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+45
-25
@@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber(
1804
return;
1805
}
1806
case OffscreenComponent: {
1807
- // If this offscreen component is hidden, we already unmounted it. Before
1808
- // deleting the children, track that it's already unmounted so that we
1809
- // don't attempt to unmount the effects again.
1810
- // TODO: If the tree is hidden, in most cases we should be able to skip
1811
- // over the nested children entirely. An exception is we haven't yet found
1812
- // the topmost host node to delete, which we already track on the stack.
1813
- // But the other case is portals, which need to be detached no matter how
1814
- // deeply they are nested. We should use a subtree flag to track whether a
1815
- // subtree includes a nested portal.
1816
- const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1817
- offscreenSubtreeWasHidden =
1818
- prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1819
- recursivelyTraverseDeletionEffects(
1820
- finishedRoot,
1821
- nearestMountedAncestor,
1822
- deletedFiber,
1823
- );
1824
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1807
+ if (
1808
+ // TODO: Remove this dead flag
1809
+ enableSuspenseLayoutEffectSemantics &&
1810
+ deletedFiber.mode & ConcurrentMode
1811
+ ) {
1812
+ // If this offscreen component is hidden, we already unmounted it. Before
1813
+ // deleting the children, track that it's already unmounted so that we
1814
+ // don't attempt to unmount the effects again.
1815
+ // TODO: If the tree is hidden, in most cases we should be able to skip
1816
+ // over the nested children entirely. An exception is we haven't yet found
1817
+ // the topmost host node to delete, which we already track on the stack.
1818
+ // But the other case is portals, which need to be detached no matter how
1819
+ // deeply they are nested. We should use a subtree flag to track whether a
1820
+ // subtree includes a nested portal.
1821
+ const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
1822
+ offscreenSubtreeWasHidden =
1823
+ prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;
1824
+ recursivelyTraverseDeletionEffects(
1825
+ finishedRoot,
1826
+ nearestMountedAncestor,
1827
+ deletedFiber,
1828
+ );
1829
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
1830
+ } else {
1831
+ recursivelyTraverseDeletionEffects(
1832
+ finishedRoot,
1833
+ nearestMountedAncestor,
1834
+ deletedFiber,
1835
+ );
1836
+ }
1837
break;
1838
}
1839
default: {
@@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber(
2250
case OffscreenComponent: {
2251
const wasHidden = current !== null && current.memoizedState !== null;
2252
2241
- // Before committing the children, track on the stack whether this
2242
- // offscreen subtree was already hidden, so that we don't unmount the
2243
- // effects again.
2244
- const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2245
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
2246
- recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2247
- offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2253
+ if (
2254
+ // TODO: Remove this dead flag
2255
+ enableSuspenseLayoutEffectSemantics &&
2256
+ finishedWork.mode & ConcurrentMode
2257
+ ) {
2258
+ // Before committing the children, track on the stack whether this
2259
+ // offscreen subtree was already hidden, so that we don't unmount the
2260
+ // effects again.
2261
+ const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2262
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
2263
+ recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2264
+ offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2265
+ } else {
2266
+ recursivelyTraverseMutationEffects(root, finishedWork, lanes);
2267
+ }
2268
2269
commitReconciliationEffects(finishedWork);
2270
packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
+49
-54
@@ -93,60 +93,6 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
93
});
94
});
95
96
- it('does not destroy layout effects twice when hidden child is removed', async () => {
97
- function ChildA({label}) {
98
- React.useLayoutEffect(() => {
99
- Scheduler.unstable_yieldValue('Did mount: ' + label);
100
- return () => {
101
- Scheduler.unstable_yieldValue('Will unmount: ' + label);
102
- };
103
- }, []);
104
- return <Text text={label} />;
105
- }
106
-
107
- function ChildB({label}) {
108
- React.useLayoutEffect(() => {
109
- Scheduler.unstable_yieldValue('Did mount: ' + label);
110
- return () => {
111
- Scheduler.unstable_yieldValue('Will unmount: ' + label);
112
- };
113
- }, []);
114
- return <Text text={label} />;
115
- }
116
-
117
- const LazyChildA = React.lazy(() => fakeImport(ChildA));
118
- const LazyChildB = React.lazy(() => fakeImport(ChildB));
119
-
120
- function Parent({swap}) {
121
- return (
122
- <React.Suspense fallback={<Text text="Loading..." />}>
123
- {swap ? <LazyChildB label="B" /> : <LazyChildA label="A" />}
124
- </React.Suspense>
125
- );
126
- }
127
-
128
- const root = ReactDOMClient.createRoot(container);
129
- act(() => {
130
- root.render(<Parent swap={false} />);
131
- });
132
- expect(Scheduler).toHaveYielded(['Loading...']);
133
-
134
- await LazyChildA;
135
- expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']);
136
- expect(container.innerHTML).toBe('A');
137
-
138
- // Swap the position of A and B
139
- ReactDOM.flushSync(() => {
140
- root.render(<Parent swap={true} />);
141
- });
142
- expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']);
143
- expect(container.innerHTML).toBe('Loading...');
144
-
145
- await LazyChildB;
146
- expect(Scheduler).toFlushAndYield(['B', 'Did mount: B']);
147
- expect(container.innerHTML).toBe('B');
148
- });
149
-
96
it('does not destroy ref cleanup twice when hidden child is removed', async () => {
97
function ChildA({label}) {
98
return (
@@ -455,4 +401,53 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
401
expect(Scheduler).toFlushAndYield([]);
402
expect(container.innerHTML).toBe('<h1>Hello</h1>');
403
});
404
+
405
+ it('regression: unmount hidden tree, in legacy mode', async () => {
406
+ // In legacy mode, when a tree suspends and switches to a fallback, the
407
+ // effects are not unmounted. So we have to unmount them during a deletion.
408
+
409
+ function Child() {
410
+ React.useLayoutEffect(() => {
411
+ Scheduler.unstable_yieldValue('Mount');
412
+ return () => {
413
+ Scheduler.unstable_yieldValue('Unmount');
414
+ };
415
+ }, []);
416
+ return <Text text="Child" />;
417
+ }
418
+
419
+ function Sibling() {
420
+ return <Text text="Sibling" />;
421
+ }
422
+ const LazySibling = React.lazy(() => fakeImport(Sibling));
423
+
424
+ function App({showMore}) {
425
+ return (
426
+ <React.Suspense fallback={<Text text="Loading..." />}>
427
+ <Child />
428
+ {showMore ? <LazySibling /> : null}
429
+ </React.Suspense>
430
+ );
431
+ }
432
+
433
+ // Initial render
434
+ ReactDOM.render(<App showMore={false} />, container);
435
+ expect(Scheduler).toHaveYielded(['Child', 'Mount']);
436
+
437
+ // Update that suspends, causing the existing tree to switches it to
438
+ // a fallback.
439
+ ReactDOM.render(<App showMore={true} />, container);
440
+ expect(Scheduler).toHaveYielded([
441
+ 'Child',
442
+ 'Loading...',
443
+
444
+ // In a concurrent root, the effect would unmount here. But this is legacy
445
+ // mode, so it doesn't.
446
+ // Unmount
447
+ ]);
448
+
449
+ // Delete the tree and unmount the effect
450
+ ReactDOM.render(null, container);
451
+ expect(Scheduler).toHaveYielded(['Unmount']);
452
+ });
453
});