diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 9fea320a69ba..8f7741f74ee1 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber( // that don't modify the stack. switch (deletedFiber.tag) { case HostComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } // Intentional fallthrough to next branch } // eslint-disable-next-line-no-fallthrough @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(deletedFiber); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - deletedFiber.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - recordLayoutEffectDuration(deletedFiber); - } else { + if (!offscreenSubtreeWasHidden) { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { safelyCallDestroy( deletedFiber, nearestMountedAncestor, destroy, ); - } + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } } } - } - effect = effect.next; - } while (effect !== firstEffect); + effect = effect.next; + } while (effect !== firstEffect); + } } } @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber( return; } case ClassComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); - const instance = deletedFiber.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - deletedFiber, - nearestMountedAncestor, - instance, - ); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); + } } recursivelyTraverseDeletionEffects( finishedRoot, @@ -1796,6 +1802,27 @@ 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; + break; + } default: { recursivelyTraverseDeletionEffects( finishedRoot, @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber( return; } 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; + commitReconciliationEffects(finishedWork); if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; if (supportsMutation) { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 2f900921f099..70b416b3a322 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1604,7 +1604,9 @@ function commitDeletionEffectsOnFiber( // that don't modify the stack. switch (deletedFiber.tag) { case HostComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + } // Intentional fallthrough to next branch } // eslint-disable-next-line-no-fallthrough @@ -1710,54 +1712,56 @@ function commitDeletionEffectsOnFiber( case ForwardRef: case MemoComponent: case SimpleMemoComponent: { - const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); - if (updateQueue !== null) { - const lastEffect = updateQueue.lastEffect; - if (lastEffect !== null) { - const firstEffect = lastEffect.next; - - let effect = firstEffect; - do { - const {destroy, tag} = effect; - if (destroy !== undefined) { - if ((tag & HookInsertion) !== NoHookEffect) { - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - } else if ((tag & HookLayout) !== NoHookEffect) { - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStarted(deletedFiber); - } - - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - deletedFiber.mode & ProfileMode - ) { - startLayoutEffectTimer(); - safelyCallDestroy( - deletedFiber, - nearestMountedAncestor, - destroy, - ); - recordLayoutEffectDuration(deletedFiber); - } else { + if (!offscreenSubtreeWasHidden) { + const updateQueue: FunctionComponentUpdateQueue | null = (deletedFiber.updateQueue: any); + if (updateQueue !== null) { + const lastEffect = updateQueue.lastEffect; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + + let effect = firstEffect; + do { + const {destroy, tag} = effect; + if (destroy !== undefined) { + if ((tag & HookInsertion) !== NoHookEffect) { safelyCallDestroy( deletedFiber, nearestMountedAncestor, destroy, ); - } + } else if ((tag & HookLayout) !== NoHookEffect) { + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStarted(deletedFiber); + } - if (enableSchedulingProfiler) { - markComponentLayoutEffectUnmountStopped(); + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + deletedFiber.mode & ProfileMode + ) { + startLayoutEffectTimer(); + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + recordLayoutEffectDuration(deletedFiber); + } else { + safelyCallDestroy( + deletedFiber, + nearestMountedAncestor, + destroy, + ); + } + + if (enableSchedulingProfiler) { + markComponentLayoutEffectUnmountStopped(); + } } } - } - effect = effect.next; - } while (effect !== firstEffect); + effect = effect.next; + } while (effect !== firstEffect); + } } } @@ -1769,14 +1773,16 @@ function commitDeletionEffectsOnFiber( return; } case ClassComponent: { - safelyDetachRef(deletedFiber, nearestMountedAncestor); - const instance = deletedFiber.stateNode; - if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount( - deletedFiber, - nearestMountedAncestor, - instance, - ); + if (!offscreenSubtreeWasHidden) { + safelyDetachRef(deletedFiber, nearestMountedAncestor); + const instance = deletedFiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + safelyCallComponentWillUnmount( + deletedFiber, + nearestMountedAncestor, + instance, + ); + } } recursivelyTraverseDeletionEffects( finishedRoot, @@ -1796,6 +1802,27 @@ 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; + break; + } default: { recursivelyTraverseDeletionEffects( finishedRoot, @@ -2203,13 +2230,21 @@ function commitMutationEffectsOnFiber( return; } 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; + commitReconciliationEffects(finishedWork); if (flags & Visibility) { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const wasHidden = current !== null && current.memoizedState !== null; const offscreenBoundary: Fiber = finishedWork; if (supportsMutation) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index bdda4f8313df..1825d41fd1da 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -1980,7 +1980,6 @@ describe('ReactSuspenseEffectsSemantics', () => { // Destroy layout and passive effects in the errored tree. 'App destroy layout', - 'ThrowsInWillUnmount componentWillUnmount', 'Text:Fallback destroy layout', 'Text:Outside destroy layout', 'Text:Inside destroy passive', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js index cb1196baffe6..00c126bb3645 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js @@ -10,18 +10,39 @@ 'use strict'; let React; +let ReactDOM; let ReactDOMClient; +let Scheduler; let act; +let container; describe('ReactSuspenseEffectsSemanticsDOM', () => { beforeEach(() => { jest.resetModules(); React = require('react'); + ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); + Scheduler = require('scheduler'); act = require('jest-react').act; + + container = document.createElement('div'); + document.body.appendChild(container); + }); + + afterEach(() => { + document.body.removeChild(container); }); + async function fakeImport(result) { + return {default: result}; + } + + function Text(props) { + Scheduler.unstable_yieldValue(props.text); + return props.text; + } + it('should not cause a cycle when combined with a render phase update', () => { let scheduleSuspendingUpdate; @@ -63,7 +84,7 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { } act(() => { - const root = ReactDOMClient.createRoot(document.createElement('div')); + const root = ReactDOMClient.createRoot(container); root.render(); }); @@ -71,4 +92,367 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => { scheduleSuspendingUpdate(); }); }); + + 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 ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + function ChildB({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + 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', 'Ref mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Ref unmount: A']); + expect(container.innerHTML).toBe( + 'ALoading...', + ); + + await LazyChildB; + expect(Scheduler).toFlushAndYield(['B', 'Ref mount: B']); + expect(container.innerHTML).toBe('B'); + }); + + it('does not call componentWillUnmount twice when hidden child is removed', async () => { + class ChildA extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + class ChildB extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + 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 layout effects twice when parent suspense 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...'); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); + + it('does not destroy ref cleanup twice when parent suspense is removed', async () => { + function ChildA({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + function ChildB({label}) { + return ( + { + if (node) { + Scheduler.unstable_yieldValue('Ref mount: ' + label); + } else { + Scheduler.unstable_yieldValue('Ref unmount: ' + label); + } + }}> + + + ); + } + + 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', 'Ref mount: A']); + expect(container.innerHTML).toBe('A'); + + // Swap the position of A and B + ReactDOM.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Loading...', 'Ref unmount: A']); + expect(container.innerHTML).toBe( + 'ALoading...', + ); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); + + it('does not call componentWillUnmount twice when parent suspense is removed', async () => { + class ChildA extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + return ; + } + } + + class ChildB extends React.Component { + componentDidMount() { + Scheduler.unstable_yieldValue('Did mount: ' + this.props.label); + } + componentWillUnmount() { + Scheduler.unstable_yieldValue('Will unmount: ' + this.props.label); + } + render() { + 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...'); + + // Destroy the whole tree, including the hidden A + ReactDOM.flushSync(() => { + root.render(

Hello

); + }); + expect(Scheduler).toFlushAndYield([]); + expect(container.innerHTML).toBe('

Hello

'); + }); });