diff --git a/compat/test/browser/suspense.test.jsx b/compat/test/browser/suspense.test.jsx index 595ca857be..e6850839ca 100644 --- a/compat/test/browser/suspense.test.jsx +++ b/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('
Suspended...
'); expect(effectSpy).toHaveBeenCalledOnce(); expect(layoutEffectSpy).toHaveBeenCalledOnce(); - return resolve().then(() => { - rerender(); - expect(effectSpy).toHaveBeenCalledOnce(); - expect(layoutEffectSpy).toHaveBeenCalledOnce(); - expect(scratch.innerHTML).to.eql(`

hi

`); - }); + await resolve(); + await new Promise(r => setTimeout(r)); + expect(effectSpy).toHaveBeenCalledOnce(); + expect(layoutEffectSpy).toHaveBeenCalledOnce(); + expect(scratch.innerHTML).to.eql(`

hi

`); }); it('should support a call to setState before rendering the fallback', () => { diff --git a/compat/test/browser/useSyncExternalStore.test.jsx b/compat/test/browser/useSyncExternalStore.test.jsx index f2acded746..5025370d1d 100644 --- a/compat/test/browser/useSyncExternalStore.test.jsx +++ b/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 diff --git a/hooks/src/index.js b/hooks/src/index.js index c56e1c68f4..4d377f88e8 100644 --- a/hooks/src/index.js +++ b/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; diff --git a/hooks/test/browser/useEffect.test.jsx b/hooks/test/browser/useEffect.test.jsx index a3c5f0b371..284bb3bf90 100644 --- a/hooks/test/browser/useEffect.test.jsx +++ b/hooks/test/browser/useEffect.test.jsx @@ -1,4 +1,4 @@ -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'; @@ -6,6 +6,8 @@ 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
; + } + + render( + <> + + + , + 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( + <> + + + , + 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); diff --git a/test-utils/src/index.js b/test-utils/src/index.js index d5b0b9be86..f885deb5e3 100644 --- a/test-utils/src/index.js +++ b/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; } }