From 80f3d88190c07c2da11b5cac58a44c3b90fbc296 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 29 Jul 2022 19:34:28 -0400 Subject: [PATCH] Mount/unmount passive effects when Offscreen visibility changes (#24977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove unnecessary try-catch from passive deletion The individual unmount calls are already wrapped in a catch block, so this outer one serves no purpose. * Extract passive unmount effects to separate functions I'm about to add a "disconnect passive effects" function that will share much of the same code as commitPassiveUnmountOnFiber. To minimize the duplicated code, I've extracted the shared parts into separate functions, similar to what I did for commitLayoutEffectOnFiber and reappearLayoutEffects. This may not save much on code size because Closure will likely inline some of it, anyway, but it makes it harder for the two paths to accidentally diverge. * Mount/unmount passive effects on hide/show This changes the behavior of Offscreen so that passive effects are unmounted when the tree is hidden, and re-mounted when the tree is revealed again. This is already how layout effects worked. In the future we will likely add an option or heuristic to only unmount the effects of a hidden tree after a delay. That way if the tree quickly switches back to visible, we can skip toggling the effects entirely. This change does not apply to suspended trees, which happen to use the Offscreen fiber type as an implementation detail. Passive effects remain mounted while the tree is suspended, for the reason described above — it's likely that the suspended tree will resolve and switch back to visible within a short time span. At a high level, what this capability enables is a feature we refer to as "resuable state". The real value proposition here isn't so much the behavior of effects — it's that you can switch back to a previously rendered tree without losing the state of the UI. * Add more coverage for nested Offscreen cases --- .../src/ReactFiberCommitWork.new.js | 240 ++++++++++------ .../src/ReactFiberCommitWork.old.js | 240 ++++++++++------ .../src/ReactFiberCompleteWork.new.js | 25 +- .../src/ReactFiberCompleteWork.old.js | 25 +- .../src/__tests__/ReactOffscreen-test.js | 263 +++++++++++++++++- 5 files changed, 616 insertions(+), 177 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 4f9b4ce42689c..3f7417fc626d8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -3553,6 +3553,60 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void { resetCurrentDebugFiberInDEV(); } +function detachAlternateSiblings(parentFiber: Fiber) { + if (deletedTreeCleanUpLevel >= 1) { + // A fiber was deleted from this parent fiber, but it's still part of the + // previous (alternate) parent fiber's list of children. Because children + // are a linked list, an earlier sibling that's still alive will be + // connected to the deleted fiber via its `alternate`: + // + // live fiber --alternate--> previous live fiber --sibling--> deleted + // fiber + // + // We can't disconnect `alternate` on nodes that haven't been deleted yet, + // but we can disconnect the `sibling` and `child` pointers. + + const previousFiber = parentFiber.alternate; + if (previousFiber !== null) { + let detachedChild = previousFiber.child; + if (detachedChild !== null) { + previousFiber.child = null; + do { + const detachedSibling = detachedChild.sibling; + detachedChild.sibling = null; + detachedChild = detachedSibling; + } while (detachedChild !== null); + } + } + } +} + +function commitHookPassiveUnmountEffects( + finishedWork: Fiber, + nearestMountedAncestor, + hookFlags: HookFlags, +) { + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + startPassiveEffectTimer(); + commitHookEffectListUnmount( + hookFlags, + finishedWork, + nearestMountedAncestor, + ); + recordPassiveEffectDuration(finishedWork); + } else { + commitHookEffectListUnmount( + hookFlags, + finishedWork, + nearestMountedAncestor, + ); + } +} + function recursivelyTraversePassiveUnmountEffects(parentFiber: Fiber): void { // Deletions effects can be scheduled on any fiber type. They need to happen // before the children effects have fired. @@ -3562,44 +3616,15 @@ function recursivelyTraversePassiveUnmountEffects(parentFiber: Fiber): void { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const childToDelete = deletions[i]; - try { - // TODO: Convert this to use recursion - nextEffect = childToDelete; - commitPassiveUnmountEffectsInsideOfDeletedTree_begin( - childToDelete, - parentFiber, - ); - } catch (error) { - captureCommitPhaseError(childToDelete, parentFiber, error); - } - } - } - - if (deletedTreeCleanUpLevel >= 1) { - // A fiber was deleted from this parent fiber, but it's still part of - // the previous (alternate) parent fiber's list of children. Because - // children are a linked list, an earlier sibling that's still alive - // will be connected to the deleted fiber via its `alternate`: - // - // live fiber - // --alternate--> previous live fiber - // --sibling--> deleted fiber - // - // We can't disconnect `alternate` on nodes that haven't been deleted - // yet, but we can disconnect the `sibling` and `child` pointers. - const previousFiber = parentFiber.alternate; - if (previousFiber !== null) { - let detachedChild = previousFiber.child; - if (detachedChild !== null) { - previousFiber.child = null; - do { - const detachedSibling = detachedChild.sibling; - detachedChild.sibling = null; - detachedChild = detachedSibling; - } while (detachedChild !== null); - } + // TODO: Convert this to use recursion + nextEffect = childToDelete; + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + childToDelete, + parentFiber, + ); } } + detachAlternateSiblings(parentFiber); } const prevDebugFiber = getCurrentDebugFiberInDEV(); @@ -3622,33 +3647,40 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { case SimpleMemoComponent: { recursivelyTraversePassiveUnmountEffects(finishedWork); if (finishedWork.flags & Passive) { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - startPassiveEffectTimer(); - commitHookEffectListUnmount( - HookPassive | HookHasEffect, - finishedWork, - finishedWork.return, - ); - recordPassiveEffectDuration(finishedWork); - } else { - commitHookEffectListUnmount( - HookPassive | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } + commitHookPassiveUnmountEffects( + finishedWork, + finishedWork.return, + HookPassive | HookHasEffect, + ); } break; } - // TODO: Disconnect passive effects when a tree is hidden, perhaps after - // a delay. - // case OffscreenComponent: { - // ... - // } + case OffscreenComponent: { + const instance: OffscreenInstance = finishedWork.stateNode; + const nextState: OffscreenState | null = finishedWork.memoizedState; + + const isHidden = nextState !== null; + + if ( + isHidden && + instance.visibility & OffscreenPassiveEffectsConnected && + // For backwards compatibility, don't unmount when a tree suspends. In + // the future we may change this to unmount after a delay. + (finishedWork.return === null || + finishedWork.return.tag !== SuspenseComponent) + ) { + // The effects are currently connected. Disconnect them. + // TODO: Add option or heuristic to delay before disconnecting the + // effects. Then if the tree reappears before the delay has elapsed, we + // can skip toggling the effects entirely. + instance.visibility &= ~OffscreenPassiveEffectsConnected; + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + } else { + recursivelyTraversePassiveUnmountEffects(finishedWork); + } + + break; + } default: { recursivelyTraversePassiveUnmountEffects(finishedWork); break; @@ -3656,6 +3688,70 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { } } +function recursivelyTraverseDisconnectPassiveEffects(parentFiber: Fiber): void { + // Deletions effects can be scheduled on any fiber type. They need to happen + // before the children effects have fired. + const deletions = parentFiber.deletions; + + if ((parentFiber.flags & ChildDeletion) !== NoFlags) { + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + // TODO: Convert this to use recursion + nextEffect = childToDelete; + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + childToDelete, + parentFiber, + ); + } + } + detachAlternateSiblings(parentFiber); + } + + const prevDebugFiber = getCurrentDebugFiberInDEV(); + // TODO: Check PassiveStatic flag + let child = parentFiber.child; + while (child !== null) { + setCurrentDebugFiberInDEV(child); + disconnectPassiveEffect(child); + child = child.sibling; + } + setCurrentDebugFiberInDEV(prevDebugFiber); +} + +function disconnectPassiveEffect(finishedWork: Fiber): void { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + // TODO: Check PassiveStatic flag + commitHookPassiveUnmountEffects( + finishedWork, + finishedWork.return, + HookPassive, + ); + // When disconnecting passive effects, we fire the effects in the same + // order as during a deletiong: parent before child + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + break; + } + case OffscreenComponent: { + const instance: OffscreenInstance = finishedWork.stateNode; + if (instance.visibility & OffscreenPassiveEffectsConnected) { + instance.visibility &= ~OffscreenPassiveEffectsConnected; + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + } else { + // The effects are already disconnected. + } + break; + } + default: { + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + break; + } + } +} + function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( deletedSubtreeRoot: Fiber, nearestMountedAncestor: Fiber | null, @@ -3728,25 +3824,11 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( case FunctionComponent: case ForwardRef: case SimpleMemoComponent: { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - current.mode & ProfileMode - ) { - startPassiveEffectTimer(); - commitHookEffectListUnmount( - HookPassive, - current, - nearestMountedAncestor, - ); - recordPassiveEffectDuration(current); - } else { - commitHookEffectListUnmount( - HookPassive, - current, - nearestMountedAncestor, - ); - } + commitHookPassiveUnmountEffects( + current, + nearestMountedAncestor, + HookPassive, + ); break; } // TODO: run passive unmount effects when unmounting a root. diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index bd16246302de8..b0e16e8ab2aeb 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -3553,6 +3553,60 @@ export function commitPassiveUnmountEffects(finishedWork: Fiber): void { resetCurrentDebugFiberInDEV(); } +function detachAlternateSiblings(parentFiber: Fiber) { + if (deletedTreeCleanUpLevel >= 1) { + // A fiber was deleted from this parent fiber, but it's still part of the + // previous (alternate) parent fiber's list of children. Because children + // are a linked list, an earlier sibling that's still alive will be + // connected to the deleted fiber via its `alternate`: + // + // live fiber --alternate--> previous live fiber --sibling--> deleted + // fiber + // + // We can't disconnect `alternate` on nodes that haven't been deleted yet, + // but we can disconnect the `sibling` and `child` pointers. + + const previousFiber = parentFiber.alternate; + if (previousFiber !== null) { + let detachedChild = previousFiber.child; + if (detachedChild !== null) { + previousFiber.child = null; + do { + const detachedSibling = detachedChild.sibling; + detachedChild.sibling = null; + detachedChild = detachedSibling; + } while (detachedChild !== null); + } + } + } +} + +function commitHookPassiveUnmountEffects( + finishedWork: Fiber, + nearestMountedAncestor, + hookFlags: HookFlags, +) { + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + startPassiveEffectTimer(); + commitHookEffectListUnmount( + hookFlags, + finishedWork, + nearestMountedAncestor, + ); + recordPassiveEffectDuration(finishedWork); + } else { + commitHookEffectListUnmount( + hookFlags, + finishedWork, + nearestMountedAncestor, + ); + } +} + function recursivelyTraversePassiveUnmountEffects(parentFiber: Fiber): void { // Deletions effects can be scheduled on any fiber type. They need to happen // before the children effects have fired. @@ -3562,44 +3616,15 @@ function recursivelyTraversePassiveUnmountEffects(parentFiber: Fiber): void { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const childToDelete = deletions[i]; - try { - // TODO: Convert this to use recursion - nextEffect = childToDelete; - commitPassiveUnmountEffectsInsideOfDeletedTree_begin( - childToDelete, - parentFiber, - ); - } catch (error) { - captureCommitPhaseError(childToDelete, parentFiber, error); - } - } - } - - if (deletedTreeCleanUpLevel >= 1) { - // A fiber was deleted from this parent fiber, but it's still part of - // the previous (alternate) parent fiber's list of children. Because - // children are a linked list, an earlier sibling that's still alive - // will be connected to the deleted fiber via its `alternate`: - // - // live fiber - // --alternate--> previous live fiber - // --sibling--> deleted fiber - // - // We can't disconnect `alternate` on nodes that haven't been deleted - // yet, but we can disconnect the `sibling` and `child` pointers. - const previousFiber = parentFiber.alternate; - if (previousFiber !== null) { - let detachedChild = previousFiber.child; - if (detachedChild !== null) { - previousFiber.child = null; - do { - const detachedSibling = detachedChild.sibling; - detachedChild.sibling = null; - detachedChild = detachedSibling; - } while (detachedChild !== null); - } + // TODO: Convert this to use recursion + nextEffect = childToDelete; + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + childToDelete, + parentFiber, + ); } } + detachAlternateSiblings(parentFiber); } const prevDebugFiber = getCurrentDebugFiberInDEV(); @@ -3622,33 +3647,40 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { case SimpleMemoComponent: { recursivelyTraversePassiveUnmountEffects(finishedWork); if (finishedWork.flags & Passive) { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - startPassiveEffectTimer(); - commitHookEffectListUnmount( - HookPassive | HookHasEffect, - finishedWork, - finishedWork.return, - ); - recordPassiveEffectDuration(finishedWork); - } else { - commitHookEffectListUnmount( - HookPassive | HookHasEffect, - finishedWork, - finishedWork.return, - ); - } + commitHookPassiveUnmountEffects( + finishedWork, + finishedWork.return, + HookPassive | HookHasEffect, + ); } break; } - // TODO: Disconnect passive effects when a tree is hidden, perhaps after - // a delay. - // case OffscreenComponent: { - // ... - // } + case OffscreenComponent: { + const instance: OffscreenInstance = finishedWork.stateNode; + const nextState: OffscreenState | null = finishedWork.memoizedState; + + const isHidden = nextState !== null; + + if ( + isHidden && + instance.visibility & OffscreenPassiveEffectsConnected && + // For backwards compatibility, don't unmount when a tree suspends. In + // the future we may change this to unmount after a delay. + (finishedWork.return === null || + finishedWork.return.tag !== SuspenseComponent) + ) { + // The effects are currently connected. Disconnect them. + // TODO: Add option or heuristic to delay before disconnecting the + // effects. Then if the tree reappears before the delay has elapsed, we + // can skip toggling the effects entirely. + instance.visibility &= ~OffscreenPassiveEffectsConnected; + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + } else { + recursivelyTraversePassiveUnmountEffects(finishedWork); + } + + break; + } default: { recursivelyTraversePassiveUnmountEffects(finishedWork); break; @@ -3656,6 +3688,70 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { } } +function recursivelyTraverseDisconnectPassiveEffects(parentFiber: Fiber): void { + // Deletions effects can be scheduled on any fiber type. They need to happen + // before the children effects have fired. + const deletions = parentFiber.deletions; + + if ((parentFiber.flags & ChildDeletion) !== NoFlags) { + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + // TODO: Convert this to use recursion + nextEffect = childToDelete; + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + childToDelete, + parentFiber, + ); + } + } + detachAlternateSiblings(parentFiber); + } + + const prevDebugFiber = getCurrentDebugFiberInDEV(); + // TODO: Check PassiveStatic flag + let child = parentFiber.child; + while (child !== null) { + setCurrentDebugFiberInDEV(child); + disconnectPassiveEffect(child); + child = child.sibling; + } + setCurrentDebugFiberInDEV(prevDebugFiber); +} + +function disconnectPassiveEffect(finishedWork: Fiber): void { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + // TODO: Check PassiveStatic flag + commitHookPassiveUnmountEffects( + finishedWork, + finishedWork.return, + HookPassive, + ); + // When disconnecting passive effects, we fire the effects in the same + // order as during a deletiong: parent before child + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + break; + } + case OffscreenComponent: { + const instance: OffscreenInstance = finishedWork.stateNode; + if (instance.visibility & OffscreenPassiveEffectsConnected) { + instance.visibility &= ~OffscreenPassiveEffectsConnected; + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + } else { + // The effects are already disconnected. + } + break; + } + default: { + recursivelyTraverseDisconnectPassiveEffects(finishedWork); + break; + } + } +} + function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( deletedSubtreeRoot: Fiber, nearestMountedAncestor: Fiber | null, @@ -3728,25 +3824,11 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber( case FunctionComponent: case ForwardRef: case SimpleMemoComponent: { - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - current.mode & ProfileMode - ) { - startPassiveEffectTimer(); - commitHookEffectListUnmount( - HookPassive, - current, - nearestMountedAncestor, - ); - recordPassiveEffectDuration(current); - } else { - commitHookEffectListUnmount( - HookPassive, - current, - nearestMountedAncestor, - ); - } + commitHookPassiveUnmountEffects( + current, + nearestMountedAncestor, + HookPassive, + ); break; } // TODO: run passive unmount effects when unmounting a root. diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 058a14d802aaa..e1d24857c93b3 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -1513,15 +1513,22 @@ function completeWork( const nextState: OffscreenState | null = workInProgress.memoizedState; const nextIsHidden = nextState !== null; - if (current !== null) { - const prevState: OffscreenState | null = current.memoizedState; - const prevIsHidden = prevState !== null; - if ( - prevIsHidden !== nextIsHidden && - // LegacyHidden doesn't do any hiding — it only pre-renders. - (!enableLegacyHidden || workInProgress.tag !== LegacyHiddenComponent) - ) { - workInProgress.flags |= Visibility; + // Schedule a Visibility effect if the visibility has changed + if (enableLegacyHidden && workInProgress.tag === LegacyHiddenComponent) { + // LegacyHidden doesn't do any hiding — it only pre-renders. + } else { + if (current !== null) { + const prevState: OffscreenState | null = current.memoizedState; + const prevIsHidden = prevState !== null; + if (prevIsHidden !== nextIsHidden) { + workInProgress.flags |= Visibility; + } + } else { + // On initial mount, we only need a Visibility effect if the tree + // is hidden. + if (nextIsHidden) { + workInProgress.flags |= Visibility; + } } } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index d21f726693dc9..10f8e43e35bd8 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -1513,15 +1513,22 @@ function completeWork( const nextState: OffscreenState | null = workInProgress.memoizedState; const nextIsHidden = nextState !== null; - if (current !== null) { - const prevState: OffscreenState | null = current.memoizedState; - const prevIsHidden = prevState !== null; - if ( - prevIsHidden !== nextIsHidden && - // LegacyHidden doesn't do any hiding — it only pre-renders. - (!enableLegacyHidden || workInProgress.tag !== LegacyHiddenComponent) - ) { - workInProgress.flags |= Visibility; + // Schedule a Visibility effect if the visibility has changed + if (enableLegacyHidden && workInProgress.tag === LegacyHiddenComponent) { + // LegacyHidden doesn't do any hiding — it only pre-renders. + } else { + if (current !== null) { + const prevState: OffscreenState | null = current.memoizedState; + const prevIsHidden = prevState !== null; + if (prevIsHidden !== nextIsHidden) { + workInProgress.flags |= Visibility; + } + } else { + // On initial mount, we only need a Visibility effect if the tree + // is hidden. + if (nextIsHidden) { + workInProgress.flags |= Visibility; + } } } diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js index 7658c3aedbdc6..4e77f831f9289 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js @@ -7,6 +7,7 @@ let Offscreen; let useState; let useLayoutEffect; let useEffect; +let useMemo; let startTransition; describe('ReactOffscreen', () => { @@ -22,6 +23,7 @@ describe('ReactOffscreen', () => { useState = React.useState; useLayoutEffect = React.useLayoutEffect; useEffect = React.useEffect; + useMemo = React.useMemo; startTransition = React.startTransition; }); @@ -939,7 +941,122 @@ describe('ReactOffscreen', () => { }); // @gate enableOffscreen - it("don't defer passive effects when prerendering in a tree whose effects are already connected", async () => { + it('passive effects are connected and disconnected when the visibility changes', async () => { + function Child({step}) { + useEffect(() => { + Scheduler.unstable_yieldValue(`Commit mount [${step}]`); + return () => { + Scheduler.unstable_yieldValue(`Commit unmount [${step}]`); + }; + }, [step]); + return ; + } + + function App({show, step}) { + return ( + + {useMemo( + () => ( + + ), + [step], + )} + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([1, 'Commit mount [1]']); + expect(root).toMatchRenderedOutput(); + + // Hide the tree. This will unmount the effect. + await act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['Commit unmount [1]']); + expect(root).toMatchRenderedOutput(