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

Jump to bottom

Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted #24400

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted #24400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfc22fc
Bug: Missing unmount when suspended tree deleted
acdlite Apr 19, 2022
ca98b30
Fix: Unmount suspended tree when it is deleted
acdlite Apr 19, 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
70 changes: 45 additions & 25 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 @@ -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
Open in desktop
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
Open in desktop
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']);
});
});