Skip to content

Commit

Permalink
Bugfix: In legacy mode, call suspended tree's unmount effects when it…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@acdlite
acdlite authored Apr 19, 2022
1 parent 2bf5eba commit ab9cdd3
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 104 deletions.
70 changes: 45 additions & 25 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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);

Expand Down
70 changes: 45 additions & 25 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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);

Expand Down
103 changes: 49 additions & 54 deletions packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Text text={label} />;
}

function ChildB({label}) {
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('Did mount: ' + label);
return () => {
Scheduler.unstable_yieldValue('Will unmount: ' + label);
};
}, []);
return <Text text={label} />;
}

const LazyChildA = React.lazy(() => fakeImport(ChildA));
const LazyChildB = React.lazy(() => fakeImport(ChildB));

function Parent({swap}) {
return (
<React.Suspense fallback={<Text text="Loading..." />}>
{swap ? <LazyChildB label="B" /> : <LazyChildA label="A" />}
</React.Suspense>
);
}

const root = ReactDOMClient.createRoot(container);
act(() => {
root.render(<Parent swap={false} />);
});
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(<Parent swap={true} />);
});
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 (
Expand Down Expand Up @@ -455,4 +401,53 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
expect(Scheduler).toFlushAndYield([]);
expect(container.innerHTML).toBe('<h1>Hello</h1>');
});

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 <Text text="Child" />;
}

function Sibling() {
return <Text text="Sibling" />;
}
const LazySibling = React.lazy(() => fakeImport(Sibling));

function App({showMore}) {
return (
<React.Suspense fallback={<Text text="Loading..." />}>
<Child />
{showMore ? <LazySibling /> : null}
</React.Suspense>
);
}

// Initial render
ReactDOM.render(<App showMore={false} />, container);
expect(Scheduler).toHaveYielded(['Child', 'Mount']);

// Update that suspends, causing the existing tree to switches it to
// a fallback.
ReactDOM.render(<App showMore={true} />, 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']);
});
});

0 comments on commit ab9cdd3

Please sign in to comment.