From ab9cdd34fb6c88d56c83882dbd011b4546878483 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 19 Apr 2022 17:09:07 -0400 Subject: [PATCH] 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 --- .../src/ReactFiberCommitWork.new.js | 70 +++++++----- .../src/ReactFiberCommitWork.old.js | 70 +++++++----- .../ReactSuspenseEffectsSemanticsDOM-test.js | 103 +++++++++--------- 3 files changed, 139 insertions(+), 104 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index dfb0308c012e5..2cb02b2a54664 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber( return; } case OffscreenComponent: { - // If this offscreen component is hidden, we already unmounted it. Before - // deleting the children, track that it's already unmounted so that we - // don't attempt to unmount the effects again. - // TODO: If the tree is hidden, in most cases we should be able to skip - // over the nested children entirely. An exception is we haven't yet found - // the topmost host node to delete, which we already track on the stack. - // But the other case is portals, which need to be detached no matter how - // deeply they are nested. We should use a subtree flag to track whether a - // subtree includes a nested portal. - const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; - offscreenSubtreeWasHidden = - prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; - recursivelyTraverseDeletionEffects( - finishedRoot, - nearestMountedAncestor, - deletedFiber, - ); - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + if ( + // TODO: Remove this dead flag + enableSuspenseLayoutEffectSemantics && + deletedFiber.mode & ConcurrentMode + ) { + // If this offscreen component is hidden, we already unmounted it. Before + // deleting the children, track that it's already unmounted so that we + // don't attempt to unmount the effects again. + // TODO: If the tree is hidden, in most cases we should be able to skip + // over the nested children entirely. An exception is we haven't yet found + // the topmost host node to delete, which we already track on the stack. + // But the other case is portals, which need to be detached no matter how + // deeply they are nested. We should use a subtree flag to track whether a + // subtree includes a nested portal. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = + prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + } else { + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + } break; } default: { @@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber( case OffscreenComponent: { const wasHidden = current !== null && current.memoizedState !== null; - // Before committing the children, track on the stack whether this - // offscreen subtree was already hidden, so that we don't unmount the - // effects again. - const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; - recursivelyTraverseMutationEffects(root, finishedWork, lanes); - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + if ( + // TODO: Remove this dead flag + enableSuspenseLayoutEffectSemantics && + finishedWork.mode & ConcurrentMode + ) { + // Before committing the children, track on the stack whether this + // offscreen subtree was already hidden, so that we don't unmount the + // effects again. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; + recursivelyTraverseMutationEffects(root, finishedWork, lanes); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + } else { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); + } commitReconciliationEffects(finishedWork); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 5853bcfe5356c..fbd3873fa54fa 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1804,24 +1804,36 @@ function commitDeletionEffectsOnFiber( return; } case OffscreenComponent: { - // If this offscreen component is hidden, we already unmounted it. Before - // deleting the children, track that it's already unmounted so that we - // don't attempt to unmount the effects again. - // TODO: If the tree is hidden, in most cases we should be able to skip - // over the nested children entirely. An exception is we haven't yet found - // the topmost host node to delete, which we already track on the stack. - // But the other case is portals, which need to be detached no matter how - // deeply they are nested. We should use a subtree flag to track whether a - // subtree includes a nested portal. - const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; - offscreenSubtreeWasHidden = - prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; - recursivelyTraverseDeletionEffects( - finishedRoot, - nearestMountedAncestor, - deletedFiber, - ); - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + if ( + // TODO: Remove this dead flag + enableSuspenseLayoutEffectSemantics && + deletedFiber.mode & ConcurrentMode + ) { + // If this offscreen component is hidden, we already unmounted it. Before + // deleting the children, track that it's already unmounted so that we + // don't attempt to unmount the effects again. + // TODO: If the tree is hidden, in most cases we should be able to skip + // over the nested children entirely. An exception is we haven't yet found + // the topmost host node to delete, which we already track on the stack. + // But the other case is portals, which need to be detached no matter how + // deeply they are nested. We should use a subtree flag to track whether a + // subtree includes a nested portal. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = + prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null; + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + } else { + recursivelyTraverseDeletionEffects( + finishedRoot, + nearestMountedAncestor, + deletedFiber, + ); + } break; } default: { @@ -2238,13 +2250,21 @@ function commitMutationEffectsOnFiber( case OffscreenComponent: { const wasHidden = current !== null && current.memoizedState !== null; - // Before committing the children, track on the stack whether this - // offscreen subtree was already hidden, so that we don't unmount the - // effects again. - const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; - recursivelyTraverseMutationEffects(root, finishedWork, lanes); - offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + if ( + // TODO: Remove this dead flag + enableSuspenseLayoutEffectSemantics && + finishedWork.mode & ConcurrentMode + ) { + // Before committing the children, track on the stack whether this + // offscreen subtree was already hidden, so that we don't unmount the + // effects again. + const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden; + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden; + recursivelyTraverseMutationEffects(root, finishedWork, lanes); + offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden; + } else { + recursivelyTraverseMutationEffects(root, finishedWork, lanes); + } commitReconciliationEffects(finishedWork); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js index 00c126bb36450..8eb89d3783ec7 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js @@ -93,60 +93,6 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { }); }); - it('does not destroy layout effects twice when hidden child is removed', async () => { - function ChildA({label}) { - React.useLayoutEffect(() => { - Scheduler.unstable_yieldValue('Did mount: ' + label); - return () => { - Scheduler.unstable_yieldValue('Will unmount: ' + label); - }; - }, []); - return ; - } - - function ChildB({label}) { - React.useLayoutEffect(() => { - Scheduler.unstable_yieldValue('Did mount: ' + label); - return () => { - Scheduler.unstable_yieldValue('Will unmount: ' + label); - }; - }, []); - return ; - } - - const LazyChildA = React.lazy(() => fakeImport(ChildA)); - const LazyChildB = React.lazy(() => fakeImport(ChildB)); - - function Parent({swap}) { - return ( - }> - {swap ? : } - - ); - } - - const root = ReactDOMClient.createRoot(container); - act(() => { - root.render(); - }); - expect(Scheduler).toHaveYielded(['Loading...']); - - await LazyChildA; - expect(Scheduler).toFlushAndYield(['A', 'Did mount: A']); - expect(container.innerHTML).toBe('A'); - - // Swap the position of A and B - ReactDOM.flushSync(() => { - root.render(); - }); - expect(Scheduler).toHaveYielded(['Loading...', 'Will unmount: A']); - expect(container.innerHTML).toBe('Loading...'); - - await LazyChildB; - expect(Scheduler).toFlushAndYield(['B', 'Did mount: B']); - expect(container.innerHTML).toBe('B'); - }); - it('does not destroy ref cleanup twice when hidden child is removed', async () => { function ChildA({label}) { return ( @@ -455,4 +401,53 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { expect(Scheduler).toFlushAndYield([]); expect(container.innerHTML).toBe('

Hello

'); }); + + it('regression: unmount hidden tree, in legacy mode', async () => { + // In legacy mode, when a tree suspends and switches to a fallback, the + // effects are not unmounted. So we have to unmount them during a deletion. + + function Child() { + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('Mount'); + return () => { + Scheduler.unstable_yieldValue('Unmount'); + }; + }, []); + return ; + } + + function Sibling() { + return ; + } + const LazySibling = React.lazy(() => fakeImport(Sibling)); + + function App({showMore}) { + return ( + }> + + {showMore ? : null} + + ); + } + + // Initial render + ReactDOM.render(, container); + expect(Scheduler).toHaveYielded(['Child', 'Mount']); + + // Update that suspends, causing the existing tree to switches it to + // a fallback. + ReactDOM.render(, container); + expect(Scheduler).toHaveYielded([ + 'Child', + 'Loading...', + + // In a concurrent root, the effect would unmount here. But this is legacy + // mode, so it doesn't. + // Unmount + ]); + + // Delete the tree and unmount the effect + ReactDOM.render(null, container); + expect(Scheduler).toHaveYielded(['Unmount']); + }); });