diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index 9fea320a69ba6..8f7741f74ee1f 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 2f900921f099d..70b416b3a3224 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 bdda4f8313dfa..1825d41fd1da6 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 cb1196baffe6a..00c126bb36450 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(