Fix effect-flushing for radix and animations

compat/test/browser/suspense.test.jsx

@@ -1,4 +1,5 @@
 import { act, setupRerender } from 'preact/test-utils';
+import { options } from 'preact';
 import React, {
 	createElement,
 	render,
@@ -19,6 +20,11 @@ import { vi } from 'vitest';
 const h = React.createElement;
 /* eslint-env browser */
 
+// The scheduler installed by preact/hooks, which flushes pending passive
+// effects before running a scheduled rerender. Captured here so tests can
+// opt back into default scheduling after `setupRerender()` replaced it.
+const defaultDebounce = options.debounceRendering;
+
 class Catcher extends Component {
 	constructor(props) {
 		super(props);
@@ -253,7 +259,7 @@ describe('suspense', () => {
 		});
 	});
 
-	it('should call effect cleanups', () => {
+	it('should call effect cleanups', async () => {
 		/** @type {(v) => void} */
 		let set;
 		const effectSpy = vi.fn();
@@ -306,18 +312,22 @@ describe('suspense', () => {
 			scratch
 		);
 
+		// Use the default scheduler: pending passive effects must flush before
+		// the rerender that suspends, so their cleanups run when the boundary
+		// unmounts the subtree. `rerender()` would bypass that scheduler.
+		options.debounceRendering = defaultDebounce;
+
 		set(true);
-		rerender();
+		await new Promise(r => setTimeout(r));
 		expect(scratch.innerHTML).to.eql('<div>Suspended...</div>');
 		expect(effectSpy).toHaveBeenCalledOnce();
 		expect(layoutEffectSpy).toHaveBeenCalledOnce();
 
-		return resolve().then(() => {
-			rerender();
-			expect(effectSpy).toHaveBeenCalledOnce();
-			expect(layoutEffectSpy).toHaveBeenCalledOnce();
-			expect(scratch.innerHTML).to.eql(`<div><p>hi</p></div>`);
-		});
+		await resolve();
+		await new Promise(r => setTimeout(r));
+		expect(effectSpy).toHaveBeenCalledOnce();
+		expect(layoutEffectSpy).toHaveBeenCalledOnce();
+		expect(scratch.innerHTML).to.eql(`<div><p>hi</p></div>`);
 	});
 
 	it('should support a call to setState before rendering the fallback', () => {

compat/test/browser/useSyncExternalStore.test.jsx

@@ -8,12 +8,18 @@ import React, {
 	useEffect,
 	useLayoutEffect
 } from 'preact/compat';
+import { options } from 'preact';
 import { setupRerender, act } from 'preact/test-utils';
 import { setupScratch, teardown } from '../../../test/_util/helpers';
 import { vi } from 'vitest';
 
 const ReactDOM = React;
 
+// The scheduler installed by preact/hooks, which flushes pending passive
+// effects before running a scheduled rerender. Captured here so tests can
+// opt back into default scheduling after `setupRerender()` replaced it.
+const defaultDebounce = options.debounceRendering;
+
 describe('useSyncExternalStore', () => {
 	/** @type {HTMLDivElement} */
 	let scratch;
@@ -476,9 +482,11 @@ describe('useSyncExternalStore', () => {
 
 			// Schedule an update. We'll intentionally not use `act` so that we can
 			// insert a mutation before React subscribes to the store in a
-			// passive effect.
+			// passive effect. Use the default scheduler so pending passive
+			// effects flush before rerenders, like they do in a real app.
+			options.debounceRendering = defaultDebounce;
 			store.set(1);
-			rerender();
+			await null;
 			assertLog([
 				1
 				// Passive effect hasn't fired yet
@@ -487,7 +495,7 @@ describe('useSyncExternalStore', () => {
 
 			// Flip the store state back to the previous value.
 			store.set(0);
-			rerender();
+			await null;
 			assertLog([
 				'Passive effect: 1',
 				// Re-render. If the current state were tracked by updating a ref in a

hooks/src/index.js

@@ -27,6 +27,14 @@ let oldAfterDiff = options.diffed;
 let oldCommit = options._commit;
 let oldBeforeUnmount = options.unmount;
 let oldRoot = options._root;
+let oldDebounce = options.debounceRendering || queueMicrotask;
+
+options.debounceRendering = callback => {
+	oldDebounce(() => {
+		flushAfterPaintEffects();
+		callback();
+	});
+};
 
 // We take the minimum timeout for requestAnimationFrame to ensure that
 // the callback is invoked after the next frame. 35ms is based on a 30hz
@@ -41,8 +49,12 @@ options._diff = vnode => {
 };
 
 options._root = (vnode, parentDom) => {
-	if (vnode && parentDom._children && parentDom._children._mask) {
-		vnode._mask = parentDom._children._mask;
+	if (vnode) {
+		flushAfterPaintEffects();
+
+		if (parentDom._children && parentDom._children._mask) {
+			vnode._mask = parentDom._children._mask;
+		}
 	}
 
 	if (oldRoot) oldRoot(vnode, parentDom);
@@ -66,11 +78,6 @@ options._render = vnode => {
 				}
 				hookItem._pendingArgs = hookItem._nextValue = undefined;
 			});
-		} else {
-			hooks._pendingEffects.some(invokeCleanup);
-			hooks._pendingEffects.some(invokeEffect);
-			hooks._pendingEffects = [];
-			currentIndex = 0;
 		}
 	}
 	previousComponent = currentComponent;

hooks/test/browser/useEffect.test.jsx

@@ -1,11 +1,13 @@
-import { Component, Fragment, createElement, render } from 'preact';
+import { Component, Fragment, createElement, options, render } from 'preact';
 import { useEffect, 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';
 import { scheduleEffectAssert } from '../_util/useEffectUtil';
 import { useEffectAssertions } from './useEffectAssertions';
 
+const debounceRendering = options.debounceRendering;
+
 describe('useEffect', () => {
 	/** @type {HTMLDivElement} */
 	let scratch;
@@ -41,6 +43,74 @@ describe('useEffect', () => {
 		expect(callback).toHaveBeenCalledTimes(2);
 	});
 
+	it('flushes pending effects before a callback ref update renders', async () => {
+		const calls = [];
+		teardownAct();
+		options.debounceRendering = debounceRendering;
+
+		function PendingEffect() {
+			useEffect(() => {
+				calls.push('effect');
+			}, []);
+			return null;
+		}
+
+		function RefUpdate() {
+			const [element, setElement] = useState(null);
+			if (element) calls.push('render');
+			return <div ref={setElement} />;
+		}
+
+		render(
+			<>
+				<PendingEffect />
+				<RefUpdate />
+			</>,
+			scratch
+		);
+
+		await new Promise(queueMicrotask);
+
+		expect(calls).to.deep.equal(['effect', 'render']);
+	});
+
+	it('flushes all pending effects before a scheduled render', async () => {
+		const calls = [];
+		let setValue;
+		teardownAct();
+		options.debounceRendering = debounceRendering;
+
+		function A() {
+			useEffect(() => {
+				calls.push('A effect');
+			}, []);
+			return null;
+		}
+
+		function B() {
+			const [value, updateValue] = useState(0);
+			setValue = updateValue;
+			useEffect(() => {
+				calls.push('B effect');
+			}, []);
+			if (value) calls.push('B render');
+			return null;
+		}
+
+		render(
+			<>
+				<A />
+				<B />
+			</>,
+			scratch
+		);
+
+		setValue(1);
+		await new Promise(queueMicrotask);
+
+		expect(calls).to.deep.equal(['A effect', 'B effect', 'B render']);
+	});
+
 	it('cancels the effect when the component get unmounted before it had the chance to run it', () => {
 		const cleanupFunction = vi.fn();
 		const callback = vi.fn(() => cleanupFunction);

test-utils/src/index.js

@@ -1,5 +1,10 @@
 import { options } from 'preact';
 
+// The scheduler in place when test-utils loads (e.g. the effect-flushing
+// scheduler installed by preact/hooks). Used as the reset value on teardown
+// so that resetting does not strip renderer-installed schedulers.
+const initialDebounce = options.debounceRendering;
+
 /**
  * Setup a rerender function that will drain the queue of pending renders
  * @returns {() => void}
@@ -121,10 +126,16 @@ export function teardown() {
 		delete options.__test__drainQueue;
 	}
 
-	if (typeof options.__test__previousDebounce != 'undefined') {
+	// The `in` check tells a saved `undefined` apart from "setupRerender was
+	// never called": act() must restore `undefined` when the scheduler was
+	// explicitly unset before it ran. Without a saved value we reset to the
+	// scheduler present at load time instead of `undefined`, so that
+	// schedulers installed by renderers (e.g. the effect-flushing scheduler
+	// from preact/hooks) survive test teardowns.
+	if ('__test__previousDebounce' in options) {
 		options.debounceRendering = options.__test__previousDebounce;
 		delete options.__test__previousDebounce;
 	} else {
-		options.debounceRendering = undefined;
+		options.debounceRendering = initialDebounce;
 	}
 }