Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mount/unmount passive effects when Offscreen visibility changes #24977

Merged
merged 4 commits into from
Jul 29, 2022
Merged

Mount/unmount passive effects when Offscreen visibility changes #24977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
069d7d7
Remove unnecessary try-catch from passive deletion
acdlite Jul 21, 2022
50af36e
Extract passive unmount effects to separate functions
acdlite Jul 21, 2022
47598d0
Mount/unmount passive effects on hide/show
acdlite Jul 22, 2022
e582972
Add more coverage for nested Offscreen cases
acdlite Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 161 additions & 79 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
Expand All @@ -3622,40 +3647,111 @@ 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this catch this case <Suspense fallback={<Offscreen>...</Offscreen>}>{...}</Suspense>?

Might be cleaner to just add another "mode" for now that user space can use too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for the internal Offscreen fiber that Suspense uses. We don't unmount passive effects when there's an avoided fallback.

Yeah I can add a "mode" for that. For the userspace API, I had imagined we might have a way to control the delay for when effects get detached, and one of the options could be "never".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this catch this case <Suspense fallback={...}>{...}?

Oh I see you're asking if there's a false positive. No because the fallback children are wrapped in an extra Fragment for reconciliation purposes.

) {
// 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;
}
}
}

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,
Expand Down Expand Up @@ -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.
Expand Down
Loading