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.

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

Call cleanup of insertion effects when hidden #30954

Merged
merged 7 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
75 changes: 74 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import type {
import {
alwaysThrottleRetries,
enableCreateEventHandleAPI,
enableHiddenSubtreeInsertionEffectCleanup,
enablePersistedModeClonedFlag,
enableProfilerTimer,
enableProfilerCommitHooks,
Expand Down Expand Up @@ -147,6 +148,7 @@ import {
getExecutionContext,
CommitContext,
NoContext,
setIsRunningInsertionEffect,
} from './ReactFiberWorkLoop';
import {
NoFlags as NoHookEffect,
Expand Down Expand Up @@ -1324,7 +1326,78 @@ function commitDeletionEffectsOnFiber(
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
if (!offscreenSubtreeWasHidden) {
if (enableHiddenSubtreeInsertionEffectCleanup) {
// When deleting a fiber, we may need to destroy insertion or layout effects.
// Insertion effects are not destroyed on hidden, only when destroyed, so now
// we need to destroy them. Layout effects are destroyed when hidden, so
// we only need to destroy them if the tree is visible.
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 tag = effect.tag;
const inst = effect.inst;
const destroy = inst.destroy;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
// TODO: add insertion effect marks and profiling.
if (__DEV__) {
setIsRunningInsertionEffect(true);
}

inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);

if (__DEV__) {
setIsRunningInsertionEffect(false);
}
} else if (
!offscreenSubtreeWasHidden &&
(tag & HookLayout) !== NoHookEffect
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
) {
// Offscreen fibers already unmounted their layout effects.
// We only need to destroy layout effects for visible trees.
if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStarted(deletedFiber);
}

if (shouldProfile(deletedFiber)) {
startLayoutEffectTimer();
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
}

if (enableSchedulingProfiler) {
markComponentLayoutEffectUnmountStopped();
}
}
}
effect = effect.next;
} while (effect !== firstEffect);
}
}
} else if (!offscreenSubtreeWasHidden) {
const updateQueue: FunctionComponentUpdateQueue | null =
(deletedFiber.updateQueue: any);
if (updateQueue !== null) {
Expand Down
85 changes: 85 additions & 0 deletions packages/react-reconciler/src/__tests__/Activity-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ let Activity;
let useState;
let useLayoutEffect;
let useEffect;
let useInsertionEffect;
let useMemo;
let useRef;
let startTransition;
Expand All @@ -25,6 +26,7 @@ describe('Activity', () => {
LegacyHidden = React.unstable_LegacyHidden;
Activity = React.unstable_Activity;
useState = React.useState;
useInsertionEffect = React.useInsertionEffect;
useLayoutEffect = React.useLayoutEffect;
useEffect = React.useEffect;
useMemo = React.useMemo;
Expand All @@ -43,6 +45,13 @@ describe('Activity', () => {
}

function LoggedText({text, children}) {
useInsertionEffect(() => {
Scheduler.log(`mount insertion ${text}`);
return () => {
Scheduler.log(`unmount insertion ${text}`);
};
});

useEffect(() => {
Scheduler.log(`mount ${text}`);
return () => {
Expand Down Expand Up @@ -1436,6 +1445,63 @@ describe('Activity', () => {
);
});

// @gate enableActivity
it('insertion effects are not disconnected when the visibility changes', async () => {
function Child({step}) {
useInsertionEffect(() => {
Scheduler.log(`Commit mount [${step}]`);
return () => {
Scheduler.log(`Commit unmount [${step}]`);
};
}, [step]);
return <Text text={step} />;
}

function App({show, step}) {
return (
<Activity mode={show ? 'visible' : 'hidden'}>
{useMemo(
() => (
<Child step={step} />
),
[step],
)}
</Activity>
);
}

const root = ReactNoop.createRoot();
await act(() => {
root.render(<App show={true} step={1} />);
});
assertLog([1, 'Commit mount [1]']);
expect(root).toMatchRenderedOutput(<span prop={1} />);

// Hide the tree. This will not unmount insertion effects.
await act(() => {
root.render(<App show={false} step={1} />);
});
assertLog([]);
expect(root).toMatchRenderedOutput(<span hidden={true} prop={1} />);

// Update.
await act(() => {
root.render(<App show={false} step={2} />);
});
// The update is pre-rendered so insertion effects are fired
assertLog([2, 'Commit unmount [1]', 'Commit mount [2]']);
expect(root).toMatchRenderedOutput(<span hidden={true} prop={2} />);

// Reveal the tree.
await act(() => {
root.render(<App show={true} step={2} />);
});
// The update doesn't render because it was already pre-rendered, and the
// insertion effect already fired.
assertLog([]);
expect(root).toMatchRenderedOutput(<span prop={2} />);
});

describe('manual interactivity', () => {
// @gate enableActivity
it('should attach ref only for mode null', async () => {
Expand Down Expand Up @@ -1904,6 +1970,9 @@ describe('Activity', () => {
'outer',
'middle',
'inner',
'mount insertion inner',
'mount insertion middle',
'mount insertion outer',
'mount layout inner',
'mount layout middle',
'mount layout outer',
Expand Down Expand Up @@ -1964,6 +2033,22 @@ describe('Activity', () => {
});

assertLog(['unmount layout inner', 'unmount inner']);

await act(() => {
root.render(null);
});

assertLog([
'unmount insertion outer',
'unmount layout outer',
'unmount insertion middle',
'unmount layout middle',
...(gate('enableHiddenSubtreeInsertionEffectCleanup')
? ['unmount insertion inner']
: []),
'unmount outer',
'unmount middle',
]);
});

// @gate enableActivity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ let resolveText;
let ReactNoop;
let Scheduler;
let Suspense;
let Activity;
let useState;
let useReducer;
let useEffect;
Expand Down Expand Up @@ -64,6 +65,7 @@ describe('ReactHooksWithNoopRenderer', () => {
useTransition = React.useTransition;
useDeferredValue = React.useDeferredValue;
Suspense = React.Suspense;
Activity = React.unstable_Activity;
ContinuousEventPriority =
require('react-reconciler/constants').ContinuousEventPriority;
if (gate(flags => flags.enableSuspenseList)) {
Expand Down Expand Up @@ -2997,6 +2999,48 @@ describe('ReactHooksWithNoopRenderer', () => {
root.render(<NotInsertion />);
});
});

// @gate enableActivity && enableHiddenSubtreeInsertionEffectCleanup
it('warns when setState is called from offscreen deleted insertion effect cleanup', async () => {
function App(props) {
const [, setX] = useState(0);
useInsertionEffect(() => {
if (props.throw) {
throw Error('No');
}
return () => {
setX(1);
};
}, [props.throw, props.foo]);
return null;
}

const root = ReactNoop.createRoot();
await act(() => {
root.render(
<Activity mode="hidden">
<App foo="hello" />
</Activity>,
);
});
await expect(async () => {
await act(() => {
root.render(<Activity mode="hidden" />);
});
}).toErrorDev(['useInsertionEffect must not schedule updates.']);

// Should not warn for regular effects after throw.
function NotInsertion() {
const [, setX] = useState(0);
useEffect(() => {
setX(1);
}, []);
return null;
}
await act(() => {
root.render(<NotInsertion />);
});
});
});

describe('useLayoutEffect', () => {
Expand Down
Loading
Loading