feat: Use onPostCommit to ensure that render/rerender and takeNext promises resolve after effects are run (#18)

As reported in #17, since React 19.1, passive effects (useEffect, and
the subscription in useSyncExternalStore) can run in a later task than
their commit.

That means that with the current approach, `render/rerender` or even
`takeNext` could trigger before all effects for that render had finished
processing.

This switches over to `onPostCommit`, an api that is not part of the
official React types, but has been around stable since React 18, based
on [this React RFC](https://github.com/reactjs/rfcs/pull/139/changes)
and implemented in react/react#17910.

React 16.8+ and 17 retain existing behaviour.

Author: phryneas

.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
 

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'),
+    },
+  }),
 })

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() {}],

src/renderHookToSnapshotStream.tsx

@@ -17,12 +17,14 @@ export interface SnapshotStream<Snapshot, Props> 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<Snapshot>
   /**
    * 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<Snapshot, Props> 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<Snapshot>
+  /** Rerenders the hook and waits for the commit's passive effects to run. */
   rerender: (rerenderCallbackProps: VoidOptionalArg<Props>) => Promise<void>
   unmount: () => void
 }

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(<Subscriber value={0} />)
+    events.emit('event')
+    await rerender(<Subscriber value={1} />)
+    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<React.SetStateAction<number>>
+      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 <Subscriber />
+      }
+
+      await stream.render(<Parent />)
+      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<React.SetStateAction<number>>
+    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(<Component />)
+    await stream.takeRender()
+    const committed = new Promise<void>(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(<Component value={0} />)
+    await Promise.all([
+      rerender(<Component value={1} />),
+      rerender(<Component value={2} />),
+    ])
+    expect(effectValue).toBe(2)
+  })
+
+  test('descendant commits without passive effects still resolve', async () => {
+    const stream = createRenderStream<{value: number}>()
+    let update: React.Dispatch<React.SetStateAction<number>>
+    function Child() {
+      const [value, setValue] = React.useState(0)
+      update = setValue
+      stream.replaceSnapshot({value})
+      return null
+    }
+    await stream.render(<Child />)
+    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<React.SetStateAction<number>>,
+      updateUntracked: React.Dispatch<React.SetStateAction<number>>
+    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(
+      <>
+        <Tracked />
+        <Untracked />
+      </>,
+    )
+    await stream.takeRender()
+    let resolved = false
+    const next = stream.takeRender().then(render => {
+      resolved = true
+      return render
+    })
+    const skipped = new Promise<void>(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<React.SetStateAction<number>>
+    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(<Component />)
+    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 <span data-testid="value">{value}</span>
+    }
+    await stream.render(<Component />)
+    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)
+    }
+  })
+})