Defer passive effect cleanup on unmount

Author: JoviDeCroock

compat/src/suspense.js

@@ -29,15 +29,15 @@ options._catchError = (error, newVNode, oldVNode, errorInfo) => {
 };
 
 const oldUnmount = options.unmount;
-options.unmount = vnode => {
+options.unmount = (vnode, parentVNode) => {
 	/** @type {import('./internal').Component} */
 	const component = vnode._component;
 	if (component) component._unmounted = true;
 	if (component && component._onResolve) {
 		component._onResolve();
 	}
 
-	if (oldUnmount) oldUnmount(vnode);
+	if (oldUnmount) oldUnmount(vnode, parentVNode);
 };
 
 function detachedClone(vnode, detachedParent, parentDom) {

hooks/src/index.js

@@ -17,6 +17,15 @@ let currentHook = 0;
 /** @type {Array<import('./internal').Component>} */
 let afterPaintEffects = [];
 
+/**
+ * Passive (useEffect) hook states of unmounted components whose cleanup is
+ * still pending. Deferred to the after-paint flush to match React, which runs
+ * passive destroys after the commit has painted. Each state's `_passive` holds
+ * the surviving component its errors should be routed to.
+ * @type {Array<import('./internal').EffectHookState>}
+ */
+let unmountCleanups = [];
+
 // Cast to use internal Options type
 const options = /** @type {import('./internal').Options} */ (_options);
 
@@ -115,16 +124,32 @@ options._commit = (vnode, commitQueue) => {
 	if (oldCommit) oldCommit(vnode, commitQueue);
 };
 
-/** @type {(vnode: import('./internal').VNode) => void} */
-options.unmount = vnode => {
-	if (oldBeforeUnmount) oldBeforeUnmount(vnode);
+/** @type {(vnode: import('./internal').VNode, parentVNode?: import('./internal').VNode) => void} */
+options.unmount = (vnode, parentVNode) => {
+	if (oldBeforeUnmount) oldBeforeUnmount(vnode, parentVNode);
 
 	const c = vnode._component;
 	if (c && c.__hooks) {
-		let hasErrored;
+		let hasErrored,
+			errorParent = parentVNode && parentVNode._parent;
+		// The removed subtree is detached (`_parent` nulled) by flush time, so
+		// grab the nearest surviving component now; its current vnode can still
+		// route deferred cleanup errors to a mounted error boundary.
+		while (errorParent && !errorParent._component) {
+			errorParent = errorParent._parent;
+		}
 		c.__hooks._list.some(s => {
 			try {
-				invokeCleanup(s);
+				// Layout cleanups have to run before the new DOM is in place, so
+				// they stay in the commit phase (see #1886). Passive cleanups run
+				// after paint, before any new passive effect (see #4299), with
+				// `_passive` repurposed to hold the error-routing component.
+				if (s._passive) {
+					s._passive = errorParent && errorParent._component;
+					afterPaint(unmountCleanups.push(s));
+				} else {
+					invokeCleanup(s);
+				}
 			} catch (e) {
 				hasErrored = e;
 			}
@@ -255,6 +280,7 @@ export function useEffect(callback, args) {
 	/** @type {import('./internal').EffectHookState} */
 	const state = getHookState(currentIndex++, 3);
 	if (!options._skipEffects && argsChanged(state._args, args)) {
+		state._passive = true;
 		state._value = callback;
 		state._pendingArgs = args;
 
@@ -419,18 +445,33 @@ export function useId() {
  */
 function flushAfterPaintEffects() {
 	let component;
-	while ((component = afterPaintEffects.shift())) {
-		const hooks = component.__hooks;
-		if (!component._parentDom || !hooks) continue;
-		try {
-			hooks._pendingEffects.some(invokeCleanup);
-			hooks._pendingEffects.some(invokeEffect);
-			hooks._pendingEffects = [];
-		} catch (e) {
-			hooks._pendingEffects = [];
-			options._catchError(e, component._vnode);
+	// The loop picks up components unmounted by an effect we just invoked, which
+	// don't necessarily schedule a flush of their own.
+	do {
+		// Unmounted components' passive cleanups run before any new passive
+		// effect, mirroring React running all destroys before any create.
+		while ((component = unmountCleanups.shift())) {
+			try {
+				invokeCleanup(component);
+			} catch (e) {
+				component = /** @type {any} */ (component._passive);
+				options._catchError(e, { _parent: component && component._vnode });
+			}
 		}
-	}
+
+		while ((component = afterPaintEffects.shift())) {
+			const hooks = component.__hooks;
+			if (!component._parentDom || !hooks) continue;
+			try {
+				hooks._pendingEffects.some(invokeCleanup);
+				hooks._pendingEffects.some(invokeEffect);
+				hooks._pendingEffects = [];
+			} catch (e) {
+				hooks._pendingEffects = [];
+				options._catchError(e, component._vnode);
+			}
+		}
+	} while (unmountCleanups.length);
 }
 
 let HAS_RAF = typeof requestAnimationFrame == 'function';

hooks/src/internal.d.ts

@@ -74,6 +74,12 @@ export interface EffectHookState extends BaseHookState {
 	_args?: unknown[];
 	_pendingArgs?: unknown[];
 	_cleanup?: Cleanup | void;
+	/**
+	 * Whether this is a passive effect (useEffect), whose unmount cleanup runs
+	 * after paint. Once unmounted it holds the surviving component that deferred
+	 * cleanup errors are routed to instead.
+	 */
+	_passive?: boolean | Component;
 }
 
 export interface MemoHookState<T = unknown> extends BaseHookState {

hooks/test/browser/useEffect.test.jsx

@@ -1,5 +1,5 @@
 import { Component, Fragment, createElement, render } from 'preact';
-import { useEffect, useRef, useState } from 'preact/hooks';
+import { useEffect, useLayoutEffect, useRef, useState } from 'preact/hooks';
 import { act, teardown as teardownAct } from 'preact/test-utils';
 import { vi } from 'vitest';
 import { setupScratch, teardown } from '../../../test/_util/helpers';
@@ -225,6 +225,52 @@ describe('useEffect', () => {
 		expect(scratch.innerHTML).to.equal('<p>Error</p>');
 	});
 
+	it('should route deferred cleanup errors after a state update', () => {
+		const spy = vi.fn();
+		let hide;
+
+		function ThrowOnUnmount() {
+			useEffect(
+				() => () => {
+					throw new Error('err');
+				},
+				[]
+			);
+			return <span>Child</span>;
+		}
+
+		function StatefulParent() {
+			const [show, setShow] = useState(true);
+			hide = () => setShow(false);
+			return <div>{show ? <ThrowOnUnmount /> : <span>Gone</span>}</div>;
+		}
+
+		class ErrorBoundary extends Component {
+			componentDidCatch(error) {
+				spy(error);
+				this.setState({ error: true });
+			}
+
+			render(props, state) {
+				return state.error ? <p>Error</p> : props.children;
+			}
+		}
+
+		act(() =>
+			render(
+				<ErrorBoundary>
+					<StatefulParent />
+				</ErrorBoundary>,
+				scratch
+			)
+		);
+		act(() => hide());
+
+		expect(spy).toHaveBeenCalledOnce();
+		expect(spy.mock.calls[0][0]).to.have.property('message', 'err');
+		expect(scratch.innerHTML).to.equal('<p>Error</p>');
+	});
+
 	it('catches errors when error is invoked during render', () => {
 		const spy = vi.fn();
 		let errored;
@@ -261,6 +307,35 @@ describe('useEffect', () => {
 		expect(scratch.innerHTML).to.equal('<p>Error</p>');
 	});
 
+	it('should flush cleanups of a root unmounted from within an effect', async () => {
+		const log = [];
+		const host = document.createElement('div');
+		const other = document.createElement('div');
+		scratch.appendChild(host);
+		scratch.appendChild(other);
+
+		function Other() {
+			useEffect(() => () => log.push('other cleanup'), []);
+			return <p>other</p>;
+		}
+
+		function Trigger() {
+			useEffect(() => {
+				log.push('trigger effect');
+				render(null, other);
+			}, []);
+			return <p>trigger</p>;
+		}
+
+		render(<Other />, other);
+		await new Promise(r => setTimeout(r, 60));
+
+		render(<Trigger />, host);
+		await new Promise(r => setTimeout(r, 60));
+
+		expect(log).to.deep.equal(['trigger effect', 'other cleanup']);
+	});
+
 	it('should allow creating a new root', () => {
 		const root = document.createElement('div');
 		const global = document.createElement('div');
@@ -665,6 +740,147 @@ describe('useEffect', () => {
 		});
 	});
 
+	it('should run cleanup of an unmounted child after the parent commits (#4299)', () => {
+		const log = [];
+
+		function Child() {
+			useEffect(() => {
+				log.push('child effect');
+				return () => {
+					log.push('child cleanup');
+				};
+			}, []);
+			return null;
+		}
+
+		function Parent({ show }) {
+			log.push('parent render');
+			useLayoutEffect(() => {
+				log.push('parent layout effect');
+			});
+			return show ? <Child /> : null;
+		}
+
+		act(() => render(<Parent show={true} />, scratch));
+		act(() => render(<Parent show={false} />, scratch));
+
+		expect(log).to.deep.equal([
+			'parent render',
+			'parent layout effect',
+			'child effect',
+			'parent render',
+			'parent layout effect',
+			'child cleanup'
+		]);
+	});
+
+	it('should run cleanups of a deep removed subtree in tree order (#4299)', () => {
+		const log = [];
+
+		const Level = ({ name, children }) => {
+			useLayoutEffect(() => () => log.push(`${name} layout cleanup`), []);
+			useEffect(() => () => log.push(`${name} passive cleanup`), []);
+			return <div class={name}>{children}</div>;
+		};
+
+		const App = ({ show }) => (
+			<section>
+				{show ? (
+					<Level name="A">
+						<Level name="B">
+							<Level name="C">
+								<span>leaf</span>
+							</Level>
+						</Level>
+					</Level>
+				) : null}
+			</section>
+		);
+
+		act(() => render(<App show />, scratch));
+		log.length = 0;
+		act(() => render(<App show={false} />, scratch));
+
+		// All layout cleanups run in the commit, then all passive ones after
+		// paint, each top-down. Matches React 19.
+		expect(log).to.deep.equal([
+			'A layout cleanup',
+			'B layout cleanup',
+			'C layout cleanup',
+			'A passive cleanup',
+			'B passive cleanup',
+			'C passive cleanup'
+		]);
+	});
+
+	it('should route a deferred cleanup error past boundaries inside the removed subtree', () => {
+		const log = [];
+
+		class Boundary extends Component {
+			componentDidCatch(err) {
+				log.push(`${this.props.name} caught: ${err.message}`);
+				this.setState({ errored: true });
+			}
+
+			render(props, state) {
+				return state.errored ? <p>{props.name} error</p> : props.children;
+			}
+		}
+
+		const Deep = () => {
+			useEffect(
+				() => () => {
+					throw new Error('deep');
+				},
+				[]
+			);
+			return <i>deep</i>;
+		};
+
+		// `Inner` is itself being removed, so it must not handle the error; the
+		// still-mounted `Outer` boundary has to.
+		const Removed = () => (
+			<Boundary name="Inner">
+				<div>
+					<Deep />
+				</div>
+			</Boundary>
+		);
+
+		const App = ({ show }) => (
+			<Boundary name="Outer">
+				<div>{show ? <Removed /> : null}</div>
+			</Boundary>
+		);
+
+		act(() => render(<App show />, scratch));
+		log.length = 0;
+		act(() => render(<App show={false} />, scratch));
+
+		expect(log).to.deep.equal(['Outer caught: deep']);
+		expect(scratch.innerHTML).to.equal('<p>Outer error</p>');
+	});
+
+	it('should run cleanups of unmounted components before new effects (#4299)', () => {
+		const log = [];
+
+		function Child({ name }) {
+			useEffect(() => {
+				log.push(`${name} effect`);
+				return () => {
+					log.push(`${name} cleanup`);
+				};
+			}, []);
+			return <p>{name}</p>;
+		}
+
+		act(() => render(<Child key="a" name="A" />, scratch));
+		log.length = 0;
+		act(() => render(<Child key="b" name="B" />, scratch));
+
+		expect(log).to.deep.equal(['A cleanup', 'B effect']);
+	});
+
 	it('should not rerun when receiving NaN on subsequent renders', () => {
 		const calls = [];
 		const Component = ({ value }) => {

hooks/test/browser/useEffectAssertions.jsx

@@ -133,8 +133,11 @@ export function useEffectAssertions(useEffect, scheduleEffectAssert) {
 		return scheduleEffectAssert(() => {
 			render(null, scratch);
 			rerender();
-			expect(cleanupFunction).toHaveBeenCalledOnce();
-		});
+		}).then(() =>
+			// Passive cleanups of unmounted components run in the after-paint
+			// flush (like React); layout cleanups have already run by now.
+			scheduleEffectAssert(() => expect(cleanupFunction).toHaveBeenCalledOnce())
+		);
 	});
 
 	it('works with closure effect callbacks capturing props', () => {