Skip to content

Commit d3d4d3a

Browse files
kassensrickhanlonii
kassens
and
rickhanlonii
authored
Call cleanup of insertion effects when hidden (#30954)
Insertion effects do not unmount when a subtree is removed while offscreen. Current behavior for an insertion effect is if the component goes - *visible -> removed:* calls insertion effect cleanup - *visible -> offscreen -> removed:* insertion effect cleanup is never called This makes it so we always call insertion effect cleanup when removing the component. Likely also fixes #26670 --------- Co-authored-by: Rick Hanlon <[email protected]>
1 parent 633a0fe commit d3d4d3a

13 files changed

+366
-5
lines changed

‎packages/react-reconciler/src/ReactFiberCommitWork.js‎

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import type {
4040
import {
4141
alwaysThrottleRetries,
4242
enableCreateEventHandleAPI,
43+
enableHiddenSubtreeInsertionEffectCleanup,
4344
enablePersistedModeClonedFlag,
4445
enableProfilerTimer,
4546
enableProfilerCommitHooks,
@@ -147,6 +148,7 @@ import {
147148
getExecutionContext,
148149
CommitContext,
149150
NoContext,
151+
setIsRunningInsertionEffect,
150152
} from './ReactFiberWorkLoop';
151153
import {
152154
NoFlags as NoHookEffect,
@@ -1324,7 +1326,78 @@ function commitDeletionEffectsOnFiber(
13241326
case ForwardRef:
13251327
case MemoComponent:
13261328
case SimpleMemoComponent: {
1327-
if (!offscreenSubtreeWasHidden) {
1329+
if (enableHiddenSubtreeInsertionEffectCleanup) {
1330+
// When deleting a fiber, we may need to destroy insertion or layout effects.
1331+
// Insertion effects are not destroyed on hidden, only when destroyed, so now
1332+
// we need to destroy them. Layout effects are destroyed when hidden, so
1333+
// we only need to destroy them if the tree is visible.
1334+
const updateQueue: FunctionComponentUpdateQueue | null =
1335+
(deletedFiber.updateQueue: any);
1336+
if (updateQueue !== null) {
1337+
const lastEffect = updateQueue.lastEffect;
1338+
if (lastEffect !== null) {
1339+
const firstEffect = lastEffect.next;
1340+
1341+
let effect = firstEffect;
1342+
do {
1343+
const tag = effect.tag;
1344+
const inst = effect.inst;
1345+
const destroy = inst.destroy;
1346+
if (destroy !== undefined) {
1347+
if ((tag & HookInsertion) !== NoHookEffect) {
1348+
// TODO: add insertion effect marks and profiling.
1349+
if (__DEV__) {
1350+
setIsRunningInsertionEffect(true);
1351+
}
1352+
1353+
inst.destroy = undefined;
1354+
safelyCallDestroy(
1355+
deletedFiber,
1356+
nearestMountedAncestor,
1357+
destroy,
1358+
);
1359+
1360+
if (__DEV__) {
1361+
setIsRunningInsertionEffect(false);
1362+
}
1363+
} else if (
1364+
!offscreenSubtreeWasHidden &&
1365+
(tag & HookLayout) !== NoHookEffect
1366+
) {
1367+
// Offscreen fibers already unmounted their layout effects.
1368+
// We only need to destroy layout effects for visible trees.
1369+
if (enableSchedulingProfiler) {
1370+
markComponentLayoutEffectUnmountStarted(deletedFiber);
1371+
}
1372+
1373+
if (shouldProfile(deletedFiber)) {
1374+
startLayoutEffectTimer();
1375+
inst.destroy = undefined;
1376+
safelyCallDestroy(
1377+
deletedFiber,
1378+
nearestMountedAncestor,
1379+
destroy,
1380+
);
1381+
recordLayoutEffectDuration(deletedFiber);
1382+
} else {
1383+
inst.destroy = undefined;
1384+
safelyCallDestroy(
1385+
deletedFiber,
1386+
nearestMountedAncestor,
1387+
destroy,
1388+
);
1389+
}
1390+
1391+
if (enableSchedulingProfiler) {
1392+
markComponentLayoutEffectUnmountStopped();
1393+
}
1394+
}
1395+
}
1396+
effect = effect.next;
1397+
} while (effect !== firstEffect);
1398+
}
1399+
}
1400+
} else if (!offscreenSubtreeWasHidden) {
13281401
const updateQueue: FunctionComponentUpdateQueue | null =
13291402
(deletedFiber.updateQueue: any);
13301403
if (updateQueue !== null) {

‎packages/react-reconciler/src/__tests__/Activity-test.js‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ let Activity;
77
let useState;
88
let useLayoutEffect;
99
let useEffect;
10+
let useInsertionEffect;
1011
let useMemo;
1112
let useRef;
1213
let startTransition;
@@ -25,6 +26,7 @@ describe('Activity', () => {
2526
LegacyHidden = React.unstable_LegacyHidden;
2627
Activity = React.unstable_Activity;
2728
useState = React.useState;
29+
useInsertionEffect = React.useInsertionEffect;
2830
useLayoutEffect = React.useLayoutEffect;
2931
useEffect = React.useEffect;
3032
useMemo = React.useMemo;
@@ -43,6 +45,13 @@ describe('Activity', () => {
4345
}
4446

4547
function LoggedText({text, children}) {
48+
useInsertionEffect(() => {
49+
Scheduler.log(`mount insertion ${text}`);
50+
return () => {
51+
Scheduler.log(`unmount insertion ${text}`);
52+
};
53+
});
54+
4655
useEffect(() => {
4756
Scheduler.log(`mount ${text}`);
4857
return () => {
@@ -1436,6 +1445,63 @@ describe('Activity', () => {
14361445
);
14371446
});
14381447

1448+
// @gate enableActivity
1449+
it('insertion effects are not disconnected when the visibility changes', async () => {
1450+
function Child({step}) {
1451+
useInsertionEffect(() => {
1452+
Scheduler.log(`Commit mount [${step}]`);
1453+
return () => {
1454+
Scheduler.log(`Commit unmount [${step}]`);
1455+
};
1456+
}, [step]);
1457+
return <Text text={step} />;
1458+
}
1459+
1460+
function App({show, step}) {
1461+
return (
1462+
<Activity mode={show ? 'visible' : 'hidden'}>
1463+
{useMemo(
1464+
() => (
1465+
<Child step={step} />
1466+
),
1467+
[step],
1468+
)}
1469+
</Activity>
1470+
);
1471+
}
1472+
1473+
const root = ReactNoop.createRoot();
1474+
await act(() => {
1475+
root.render(<App show={true} step={1} />);
1476+
});
1477+
assertLog([1, 'Commit mount [1]']);
1478+
expect(root).toMatchRenderedOutput(<span prop={1} />);
1479+
1480+
// Hide the tree. This will not unmount insertion effects.
1481+
await act(() => {
1482+
root.render(<App show={false} step={1} />);
1483+
});
1484+
assertLog([]);
1485+
expect(root).toMatchRenderedOutput(<span hidden={true} prop={1} />);
1486+
1487+
// Update.
1488+
await act(() => {
1489+
root.render(<App show={false} step={2} />);
1490+
});
1491+
// The update is pre-rendered so insertion effects are fired
1492+
assertLog([2, 'Commit unmount [1]', 'Commit mount [2]']);
1493+
expect(root).toMatchRenderedOutput(<span hidden={true} prop={2} />);
1494+
1495+
// Reveal the tree.
1496+
await act(() => {
1497+
root.render(<App show={true} step={2} />);
1498+
});
1499+
// The update doesn't render because it was already pre-rendered, and the
1500+
// insertion effect already fired.
1501+
assertLog([]);
1502+
expect(root).toMatchRenderedOutput(<span prop={2} />);
1503+
});
1504+
14391505
describe('manual interactivity', () => {
14401506
// @gate enableActivity
14411507
it('should attach ref only for mode null', async () => {
@@ -1904,6 +1970,9 @@ describe('Activity', () => {
19041970
'outer',
19051971
'middle',
19061972
'inner',
1973+
'mount insertion inner',
1974+
'mount insertion middle',
1975+
'mount insertion outer',
19071976
'mount layout inner',
19081977
'mount layout middle',
19091978
'mount layout outer',
@@ -1964,6 +2033,22 @@ describe('Activity', () => {
19642033
});
19652034

19662035
assertLog(['unmount layout inner', 'unmount inner']);
2036+
2037+
await act(() => {
2038+
root.render(null);
2039+
});
2040+
2041+
assertLog([
2042+
'unmount insertion outer',
2043+
'unmount layout outer',
2044+
'unmount insertion middle',
2045+
'unmount layout middle',
2046+
...(gate('enableHiddenSubtreeInsertionEffectCleanup')
2047+
? ['unmount insertion inner']
2048+
: []),
2049+
'unmount outer',
2050+
'unmount middle',
2051+
]);
19672052
});
19682053

19692054
// @gate enableActivity

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ let resolveText;
1919
let ReactNoop;
2020
let Scheduler;
2121
let Suspense;
22+
let Activity;
2223
let useState;
2324
let useReducer;
2425
let useEffect;
@@ -64,6 +65,7 @@ describe('ReactHooksWithNoopRenderer', () => {
6465
useTransition = React.useTransition;
6566
useDeferredValue = React.useDeferredValue;
6667
Suspense = React.Suspense;
68+
Activity = React.unstable_Activity;
6769
ContinuousEventPriority =
6870
require('react-reconciler/constants').ContinuousEventPriority;
6971
if (gate(flags => flags.enableSuspenseList)) {
@@ -2997,6 +2999,57 @@ describe('ReactHooksWithNoopRenderer', () => {
29972999
root.render(<NotInsertion />);
29983000
});
29993001
});
3002+
3003+
// @gate enableActivity
3004+
it('warns when setState is called from offscreen deleted insertion effect cleanup', async () => {
3005+
function App(props) {
3006+
const [, setX] = useState(0);
3007+
useInsertionEffect(() => {
3008+
if (props.throw) {
3009+
throw Error('No');
3010+
}
3011+
return () => {
3012+
setX(1);
3013+
};
3014+
}, [props.throw, props.foo]);
3015+
return null;
3016+
}
3017+
3018+
const root = ReactNoop.createRoot();
3019+
await act(() => {
3020+
root.render(
3021+
<Activity mode="hidden">
3022+
<App foo="hello" />
3023+
</Activity>,
3024+
);
3025+
});
3026+
3027+
if (gate(flags => flags.enableHiddenSubtreeInsertionEffectCleanup)) {
3028+
await expect(async () => {
3029+
await act(() => {
3030+
root.render(<Activity mode="hidden" />);
3031+
});
3032+
}).toErrorDev(['useInsertionEffect must not schedule updates.']);
3033+
} else {
3034+
await expect(async () => {
3035+
await act(() => {
3036+
root.render(<Activity mode="hidden" />);
3037+
});
3038+
}).toErrorDev([]);
3039+
}
3040+
3041+
// Should not warn for regular effects after throw.
3042+
function NotInsertion() {
3043+
const [, setX] = useState(0);
3044+
useEffect(() => {
3045+
setX(1);
3046+
}, []);
3047+
return null;
3048+
}
3049+
await act(() => {
3050+
root.render(<NotInsertion />);
3051+
});
3052+
});
30003053
});
30013054

30023055
describe('useLayoutEffect', () => {

0 commit comments

Comments
 (0)