diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9335171c..5db57b97c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,8 +10,12 @@ on: jobs: ci: - name: CI + name: CI (React ${{ matrix.react }}) runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + react: ['16.x', '17.x', '18.x', '19.0', '19.x'] steps: - name: Checkout repo uses: actions/checkout@v4 @@ -25,6 +29,15 @@ jobs: - name: Install dependencies run: yarn install + - name: Install React ${{ matrix.react }} + run: + yarn add --dev react@${{ matrix.react }} react-dom@${{ matrix.react }} + env: + YARN_ENABLE_IMMUTABLE_INSTALLS: false + + - name: Build package + run: yarn build + - name: Run tests, lint and verify package integrity run: yarn run validate diff --git a/jest.config.cjs b/jest.config.cjs index acc6fdd84..89c296e14 100644 --- a/jest.config.cjs +++ b/jest.config.cjs @@ -3,4 +3,11 @@ const {jest: jestConfig} = require('kcd-scripts/config') module.exports = Object.assign(jestConfig, { resolver: 'ts-jest-resolver', prettierPath: null, + // React 16/17 use the legacy renderer and never call the client APIs. + ...(/^(16|17)\./.test(require('react').version) && { + moduleNameMapper: { + ...jestConfig.moduleNameMapper, + '^react-dom/client$': require.resolve('react-dom'), + }, + }), }) diff --git a/src/__tests__/renderHookToSnapshotStream.test.tsx b/src/__tests__/renderHookToSnapshotStream.test.tsx index c42f3a10a..4fde8b68c 100644 --- a/src/__tests__/renderHookToSnapshotStream.test.tsx +++ b/src/__tests__/renderHookToSnapshotStream.test.tsx @@ -9,6 +9,9 @@ import { } from '@testing-library/react-render-stream' import * as React from 'react' +const testWithExternalStore = + typeof React.useSyncExternalStore === 'function' ? test : test.skip + const testEvents = new EventEmitter<{ rerenderWithValue: [unknown] }>() @@ -32,7 +35,7 @@ function useRerenderEvents(initialValue: unknown) { ) } -test('basic functionality', async () => { +testWithExternalStore('basic functionality', async () => { const {takeSnapshot} = await renderHookToSnapshotStream(useRerenderEvents, { initialProps: 'initial', }) @@ -53,7 +56,9 @@ test('basic functionality', async () => { } }) -test.each<[type: string, initialValue: unknown, ...nextValues: unknown[]]>([ +testWithExternalStore.each< + [type: string, initialValue: unknown, ...nextValues: unknown[]] +>([ ['string', 'initial', 'value', 'value2'], ['number', 0, 1, 2], ['functions', () => {}, () => {}, function named() {}], diff --git a/src/renderHookToSnapshotStream.tsx b/src/renderHookToSnapshotStream.tsx index c911232de..64b520eb6 100644 --- a/src/renderHookToSnapshotStream.tsx +++ b/src/renderHookToSnapshotStream.tsx @@ -17,12 +17,14 @@ export interface SnapshotStream extends Assertable { /** * Peeks the next render from the current iterator position, without advancing the iterator. * If no render has happened yet, it will wait for the next render to happen. + * Resolves after the recorded commit's passive effects have run. * @throws {WaitForRenderTimeoutError} if no render happens within the timeout */ peekSnapshot(options?: NextRenderOptions): Promise /** * Iterates to the next render and returns it. * If no render has happened yet, it will wait for the next render to happen. + * Resolves after the recorded commit's passive effects have run. * @throws {WaitForRenderTimeoutError} if no render happens within the timeout */ takeSnapshot: Assertable & @@ -38,9 +40,11 @@ export interface SnapshotStream extends Assertable { getCurrentSnapshot(): Snapshot /** * Waits for the next render to happen. + * Resolves after its passive effects have run. * Does not advance the render iterator. */ waitForNextSnapshot(options?: NextRenderOptions): Promise + /** Rerenders the hook and waits for the commit's passive effects to run. */ rerender: (rerenderCallbackProps: VoidOptionalArg) => Promise unmount: () => void } diff --git a/src/renderStream/__tests__/passiveEffects.test.tsx b/src/renderStream/__tests__/passiveEffects.test.tsx new file mode 100644 index 000000000..b2fe9f9d9 --- /dev/null +++ b/src/renderStream/__tests__/passiveEffects.test.tsx @@ -0,0 +1,232 @@ +import {EventEmitter} from 'node:events' +import {describe, expect, test} from '@jest/globals' +import { + createRenderStream, + useTrackRenders, +} from '@testing-library/react-render-stream' +import * as React from 'react' + +describe('passive effects', () => { + test('render and rerender wait for subscriptions without consuming renders', async () => { + const events = new EventEmitter() + const received: number[] = [] + const stream = createRenderStream<{value: number}>() + function Subscriber({value}: {value: number}) { + stream.replaceSnapshot({value}) + React.useEffect(() => { + const listener = () => received.push(value) + events.on('event', listener) + return () => { + events.off('event', listener) + } + }, [value]) + return null + } + + const {rerender} = await stream.render() + events.emit('event') + await rerender() + events.emit('event') + + expect(received).toEqual([0, 1]) + expect((await stream.takeRender()).snapshot).toEqual({value: 0}) + expect((await stream.takeRender()).snapshot).toEqual({value: 1}) + }) + + test.each(['takeRender', 'peekRender', 'waitForNextRender'] as const)( + '%s waits for effects of descendant-only updates', + async method => { + const events = new EventEmitter() + const received: number[] = [] + const stream = createRenderStream<{value: number}>() + let update: React.Dispatch> + let parentCommits = 0 + function Subscriber() { + const [value, setValue] = React.useState(0) + update = setValue + stream.replaceSnapshot({value}) + React.useEffect(() => { + const listener = () => received.push(value) + events.on('event', listener) + return () => { + events.off('event', listener) + } + }, [value]) + return null + } + function Parent() { + React.useLayoutEffect(() => { + parentCommits++ + }) + return + } + + await stream.render() + await stream.takeRender() + update!(1) + const render = await stream[method]() + events.emit('event') + + expect(render.snapshot).toEqual({value: 1}) + expect(received).toEqual([1]) + expect(parentCommits).toBe(1) + }, + ) + + test('takeRender waits for effects even when onRender has already run', async () => { + let commitRecorded: () => void = () => {} + const stream = createRenderStream<{value: number}>({ + onRender: () => commitRecorded(), + }) + let update: React.Dispatch> + let effectValue = -1 + function Component() { + const [value, setValue] = React.useState(0) + update = setValue + stream.replaceSnapshot({value}) + React.useEffect(() => { + effectValue = value + }, [value]) + return null + } + + await stream.render() + await stream.takeRender() + const committed = new Promise(resolve => { + commitRecorded = resolve + }) + update!(1) + await committed + expect((await stream.takeRender()).snapshot).toEqual({value: 1}) + expect(effectValue).toBe(1) + }) + + test('overlapping rerenders both resolve after the batched commit', async () => { + const stream = createRenderStream() + let effectValue = -1 + function Component({value}: {value: number}) { + React.useEffect(() => { + effectValue = value + }, [value]) + return null + } + const {rerender} = await stream.render() + await Promise.all([ + rerender(), + rerender(), + ]) + expect(effectValue).toBe(2) + }) + + test('descendant commits without passive effects still resolve', async () => { + const stream = createRenderStream<{value: number}>() + let update: React.Dispatch> + function Child() { + const [value, setValue] = React.useState(0) + update = setValue + stream.replaceSnapshot({value}) + return null + } + await stream.render() + await stream.takeRender() + update!(1) + expect((await stream.takeRender()).snapshot).toEqual({value: 1}) + }) + + test('skipped commits do not resolve a waiter for a tracked render', async () => { + const stream = createRenderStream<{value: number}>({ + skipNonTrackingRenders: true, + }) + let updateTracked: React.Dispatch>, + updateUntracked: React.Dispatch> + let untrackedEffect: () => void = () => {} + function Tracked() { + useTrackRenders() + const [value, setValue] = React.useState(0) + updateTracked = setValue + stream.replaceSnapshot({value}) + return null + } + function Untracked() { + const [value, setValue] = React.useState(0) + updateUntracked = setValue + React.useEffect(() => untrackedEffect(), [value]) + return null + } + await stream.render( + <> + + + , + ) + await stream.takeRender() + let resolved = false + const next = stream.takeRender().then(render => { + resolved = true + return render + }) + const skipped = new Promise(resolve => { + untrackedEffect = resolve + }) + updateUntracked!(1) + await skipped + expect(resolved).toBe(false) + updateTracked!(1) + expect((await next).snapshot).toEqual({value: 1}) + }) + + test('snapshot errors reject pending consumers after effects and preserve iteration', async () => { + const error = new Error('snapshot failed') + const stream = createRenderStream<{value: number}>({ + onRender({snapshot}) { + if (snapshot.value === 1) throw error + }, + }) + let update: React.Dispatch> + let effectValue = -1 + function Component() { + const [value, setValue] = React.useState(0) + update = setValue + stream.replaceSnapshot({value}) + React.useEffect(() => { + effectValue = value + }, [value]) + return null + } + await stream.render() + await stream.takeRender() + update!(1) + await expect(stream.takeRender()).rejects.toBe(error) + expect(effectValue).toBe(1) + update!(2) + expect((await stream.takeRender()).snapshot).toEqual({value: 2}) + }) + + test('retains commit-time snapshots across layout and passive effect updates', async () => { + const stream = createRenderStream<{value: number}>({snapshotDOM: true}) + const effects: number[] = [] + function Component() { + const [value, setValue] = React.useState(0) + stream.replaceSnapshot({value}) + React.useLayoutEffect(() => { + if (value === 0) setValue(1) + }, [value]) + React.useEffect(() => { + effects.push(value) + if (value === 1) setValue(2) + }, [value]) + return {value} + } + await stream.render() + for (const value of [0, 1, 2]) { + // eslint-disable-next-line no-await-in-loop + const render = await stream.takeRender() + expect(render.count).toBe(value + 1) + expect(render.snapshot).toEqual({value}) + expect(render.withinDOM().getByTestId('value').textContent).toBe( + String(value), + ) + expect(effects).toContain(value) + } + }) +}) diff --git a/src/renderStream/__tests__/skippedRenders.test.tsx b/src/renderStream/__tests__/skippedRenders.test.tsx new file mode 100644 index 000000000..82015fad6 --- /dev/null +++ b/src/renderStream/__tests__/skippedRenders.test.tsx @@ -0,0 +1,68 @@ +import {expect, test} from '@jest/globals' +import { + createRenderStream, + useTrackRenders, + WaitForRenderTimeoutError, +} from '@testing-library/react-render-stream' +import * as React from 'react' + +test.each(['render', 'rerender'] as const)( + '%s completes a skipped commit without notifying snapshot consumers', + async method => { + const stream = createRenderStream({skipNonTrackingRenders: true}) + let subscribed = false + function Tracked() { + useTrackRenders() + React.useEffect(() => { + subscribed = true + return () => { + subscribed = false + } + }, []) + return tracked + } + const container = document.createElement('div') + const {rerender} = await stream.render(, {container}) + await stream.takeRender() + const render = (ui: React.ReactNode) => + method === 'render' ? stream.render(ui, {container}) : rerender(ui) + let notified = false + const next = stream.takeRender().then(result => { + notified = true + return result + }) + + await render(null) + expect(container.innerHTML).toBe('') + expect(subscribed).toBe(false) + expect(stream.totalRenderCount()).toBe(1) + expect(notified).toBe(false) + + await render() + expect(subscribed).toBe(true) + expect((await next).count).toBe(2) + }, +) + +test('initial and overlapping skipped renders finish while snapshot waits still time out', async () => { + const stream = createRenderStream({skipNonTrackingRenders: true}) + let effectValue = -1 + function Untracked({value}: {value: number}) { + React.useEffect(() => { + effectValue = value + }, [value]) + return null + } + const {rerender} = await stream.render() + expect(effectValue).toBe(0) + const timeout = expect( + stream.takeRender({timeout: 100}), + ).rejects.toBeInstanceOf(WaitForRenderTimeoutError) + await Promise.all([ + rerender(), + rerender(), + ]) + expect(effectValue).toBe(2) + expect(stream.totalRenderCount()).toBe(0) + await timeout +}) diff --git a/src/renderStream/__tests__/waitForNextRender.test.tsx b/src/renderStream/__tests__/waitForNextRender.test.tsx new file mode 100644 index 000000000..137dae385 --- /dev/null +++ b/src/renderStream/__tests__/waitForNextRender.test.tsx @@ -0,0 +1,45 @@ +import {expect, jest, test} from '@jest/globals' +import {createRenderStream} from '@testing-library/react-render-stream' +import * as React from 'react' +import {flushSync} from 'react-dom' + +test.each([false, true])( + 'a completed wait cannot clear a later waiter (snapshot error: %s)', + async snapshotError => { + const error = new Error('snapshot failed') + const stream = createRenderStream<{value: number}>({ + onRender({snapshot}) { + if (snapshotError && snapshot.value === 1) throw error + }, + }) + let update: React.Dispatch> + function Component() { + const [value, setValue] = React.useState(0) + update = setValue + stream.replaceSnapshot({value}) + return null + } + await stream.render() + await stream.takeRender() + + jest.useFakeTimers() + try { + const first = stream.takeRender({timeout: 100}).then( + () => undefined, + failure => failure, + ) + flushSync(() => update!(1)) + await jest.advanceTimersByTimeAsync(0) + expect(await first).toBe(snapshotError ? error : undefined) + + const second = stream.takeRender({timeout: 200}).catch(failure => failure) + // Cross the first wait's original deadline while the second is pending. + await jest.advanceTimersByTimeAsync(100) + flushSync(() => update!(2)) + await jest.advanceTimersByTimeAsync(100) + expect(await second).toMatchObject({snapshot: {value: 2}}) + } finally { + jest.useRealTimers() + } + }, +) diff --git a/src/renderStream/createRenderStream.tsx b/src/renderStream/createRenderStream.tsx index e6518146d..6fd21738b 100644 --- a/src/renderStream/createRenderStream.tsx +++ b/src/renderStream/createRenderStream.tsx @@ -52,12 +52,14 @@ export interface RenderStream< /** * Peeks the next render from the current iterator position, without advancing the iterator. * If no render has happened yet, it will wait for the next render to happen. + * Resolves after the recorded commit's passive effects have run. * @throws {WaitForRenderTimeoutError} if no render happens within the timeout */ peekRender: (options?: NextRenderOptions) => Promise> /** * Iterates to the next render and returns it. * If no render has happened yet, it will wait for the next render to happen. + * Resolves after the recorded commit's passive effects have run. * @throws {WaitForRenderTimeoutError} if no render happens within the timeout */ takeRender: Assertable & @@ -73,6 +75,7 @@ export interface RenderStream< getCurrentRender: () => Render /** * Waits for the next render to happen. + * Resolves after its passive effects have run. * Does not advance the render iterator. */ waitForNextRender: ( @@ -84,6 +87,7 @@ export interface RenderStreamWithRenderFn< Snapshot extends ValidSnapshot, Q extends Queries = SyncQueries, > extends RenderStream { + /** Renders the UI and waits for the commit's passive effects to run. */ render: RenderWithoutActAsync } @@ -134,14 +138,20 @@ export function createRenderStream< // created, which is important for the `markAssertable` function const stream = {} as any as RenderStreamWithRenderFn + type Commit = PromiseWithResolvers< + (typeof stream.renders)[number] | undefined + > & { + finalize?: () => (typeof stream.renders)[number] + } let nextRender: Promise> | undefined, - resolveNextRender: ((render: Render) => void) | undefined, - rejectNextRender: ((error: unknown) => void) | undefined - function resetNextRender() { - nextRender = undefined - resolveNextRender = undefined - rejectNextRender = undefined + upcomingCommit: Commit | undefined + const pendingCommits: Array<{commitTime: number; commit: Commit}> = [] + + function nextCommit(): Commit { + return (upcomingCommit ??= + Promise.withResolvers>()) } + const snapshotRef = {current: initialSnapshot} const replaceSnapshot: ReplaceSnapshot = snap => { if (typeof snap === 'function') { @@ -182,6 +192,16 @@ export function createRenderStream< startTime, commitTime, ) => { + const commit = nextCommit() + upcomingCommit = undefined + pendingCommits.push({commitTime, commit}) + if (/^(16|17)\./.test(React.version)) { + // React 16/17 have no onPostCommit hook. Let the commit finish scheduling + // passive effects before yielding a task for them to run. + void Promise.resolve().then(() => { + setTimeout(() => profilerOnPostCommit(id, phase, 0, commitTime), 0) + }) + } if ( skipNonTrackingRenders && renderStreamContext.renderedComponents.length === 0 @@ -196,7 +216,10 @@ export function createRenderStream< baseDuration, startTime, commitTime, - count: stream.renders.length + 1, + count: + stream.renders.length + + pendingCommits.filter(pending => pending.commit.finalize).length + + 1, } try { /* @@ -225,25 +248,47 @@ export function createRenderStream< queries, ) renderStreamContext.renderedComponents = [] - stream.renders.push(render) - resolveNextRender?.(render) + commit.finalize = () => { + stream.renders.push(render) + return render + } } catch (error) { - stream.renders.push({ - phase: 'snapshotError', - count: stream.renders.length, - error, - }) - rejectNextRender?.(error) - } finally { - resetNextRender() + commit.finalize = () => { + const err = { + phase: 'snapshotError' as const, + count: stream.renders.length, + error, + } + stream.renders.push(err) + return err + } } } + function profilerOnPostCommit( + _id: string, + _phase: string, + _passiveEffectDuration: number, + commitTime: number, + ) { + const index = pendingCommits.findIndex( + pending => pending.commitTime === commitTime, + ) + if (index === -1) return + const {commit} = pendingCommits.splice(index, 1)[0] + commit.resolve(commit.finalize?.()) + } + let iteratorPosition = 0 function Wrapper({children}: {children: React.ReactNode}) { return ( - + {children} @@ -254,6 +299,7 @@ export function createRenderStream< ui: React.ReactNode, options?: RenderOptions, ) => { + const commit = nextCommit() const ret = await renderWithoutAct(ui, { ...options, wrapper: props => { @@ -265,21 +311,12 @@ export function createRenderStream< ) }, }) - if (stream.renders.length === 0) { - await stream.waitForNextRender() - } + await waitForCommit(commit) const origRerender = ret.rerender ret.rerender = async function rerender(rerenderUi: React.ReactNode) { - const previousRenderCount = stream.renders.length - try { - return await origRerender(rerenderUi) - } finally { - // only wait for the next render if the rerender was not - // synchronous (React 17) - if (previousRenderCount === stream.renders.length) { - await stream.waitForNextRender() - } - } + const rerenderCommit = nextCommit() + await origRerender(rerenderUi) + await waitForCommit(rerenderCommit) } return ret }) as unknown as RenderWithoutActAsync // TODO @@ -295,27 +332,18 @@ export function createRenderStream< return stream.renders.length }, async peekRender(options: NextRenderOptions = {}) { - try { - if (iteratorPosition < stream.renders.length) { - const peekedRender = stream.renders[iteratorPosition] - - if (peekedRender.phase === 'snapshotError') { - throw peekedRender.error - } + if (iteratorPosition < stream.renders.length) { + const peekedRender = stream.renders[iteratorPosition] - return peekedRender + if (peekedRender.phase === 'snapshotError') { + throw peekedRender.error } - return await stream - .waitForNextRender(options) - .catch(rethrowWithCapturedStackTrace(stream.peekRender)) - } finally { - /** drain microtask queue */ - await new Promise(resolve => { - setTimeout(() => { - resolve() - }, 0) - }) + + return peekedRender } + return stream + .waitForNextRender(options) + .catch(rethrowWithCapturedStackTrace(stream.peekRender)) }, takeRender: markAssertable(async function takeRender( options: NextRenderOptions = {}, @@ -361,26 +389,59 @@ export function createRenderStream< }, waitForNextRender({timeout = 1000}: NextRenderOptions = {}) { if (!nextRender) { + const position = stream.renders.length + const wait = {active: true} + let timer: ReturnType | undefined nextRender = Promise.race>([ - new Promise>((resolve, reject) => { - resolveNextRender = resolve - rejectNextRender = reject - }), - new Promise>((_, reject) => - setTimeout(() => { + (async () => { + while (wait.active && stream.renders.length === position) { + // A skipped commit completes without publishing a snapshot. + // eslint-disable-next-line no-await-in-loop + await (pendingCommits[0]?.commit ?? nextCommit()).promise + } + if (!wait.active) throw new WaitForRenderTimeoutError() + const next = stream.renders[position] + if (next.phase === 'snapshotError') throw next.error + return next + })(), + new Promise>((_, reject) => { + timer = setTimeout(() => { const error = new WaitForRenderTimeoutError() Error.captureStackTrace(error, stream.waitForNextRender) reject(error) - resetNextRender() - }, timeout), - ), - ]) + }, timeout) + }), + ]).finally(() => { + wait.active = false + clearTimeout(timer) + nextRender = undefined + }) } return nextRender }, render, }) return stream + + async function waitForCommit(commit: Commit) { + let timer: ReturnType | undefined + try { + return await Promise.race([ + commit.promise.then(result => { + if (result?.phase === 'snapshotError') throw result.error + return result + }), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new WaitForRenderTimeoutError()), + 1000, + ) + }), + ]) + } finally { + clearTimeout(timer) + } + } } function rethrowWithCapturedStackTrace(constructorOpt: Function | undefined) { diff --git a/src/renderWithoutAct.tsx b/src/renderWithoutAct.tsx index 26e9d3cb5..866997869 100644 --- a/src/renderWithoutAct.tsx +++ b/src/renderWithoutAct.tsx @@ -18,9 +18,9 @@ import { // Ideally we'd just use a WeakMap where containers are keys and roots are values. // We use two variables so that we can bail out in constant time when we render with a new container (most common use case) -const mountedContainers: Set = new Set() +const mountedContainers: Set = new Set() const mountedRootEntries: Array<{ - container: import('react-dom').Container + container: ReactDOMClient.Container root: ReturnType }> = [] @@ -188,12 +188,17 @@ async function _renderWithoutAct( } function createLegacyRoot(container: ReactDOMClient.Container) { + // These runtime APIs are absent from React 19's types. + const legacyReactDOM = ReactDOM as unknown as { + render(element: React.ReactNode, container: ReactDOMClient.Container): void + unmountComponentAtNode(container: ReactDOMClient.Container): boolean + } return { render(element: React.ReactNode) { - ReactDOM.render(element as unknown as React.ReactElement, container) + legacyReactDOM.render(element, container) }, unmount() { - ReactDOM.unmountComponentAtNode(container) + legacyReactDOM.unmountComponentAtNode(container) }, } } diff --git a/tests/polyfill.js b/tests/polyfill.js index bcd8e511b..c7614881a 100644 --- a/tests/polyfill.js +++ b/tests/polyfill.js @@ -3,4 +3,4 @@ import {TextEncoder, TextDecoder} from 'util' global.TextEncoder = TextEncoder global.TextDecoder = TextDecoder -Symbol.dispose = Symbol('Symbol.dispose') +if (!Symbol.dispose) Symbol.dispose = Symbol('Symbol.dispose')