Skip to content

ci: run the test suite against React 19 (#789) - #794

Open
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:ci/react-19-test-job
Open

ci: run the test suite against React 19 (#789)#794
tyler-reitz wants to merge 1 commit into
FirebaseExtended:mainfrom
tyler-reitz:ci/react-19-test-job

Conversation

@tyler-reitz

Copy link
Copy Markdown
Contributor

Why

CI type-checks React 19 but has never executed a test against it: the react matrix exists only on the type-check job, while the test job runs the lockfile's React 18. This adds React 18/19 as a test-job dimension so the suite actually runs on 19, before any src/ change lands for the React 19 foundation.

It found a real bug in its first run, which is the argument for landing this leg first: #793.

What

  • test job matrix gains react: ["18", "19"], so 2 jobs become 4. The 18 legs run the lockfile as-is; the 19 legs install react@19 react-dom@19 --no-save.
  • The 18 legs keep their existing job names, so main's required status checks keep matching and no admin change is needed. Only the 19 legs get a suffix. A consequence worth stating: a React 19 failure will not gate a merge while 19 support is still being built. Add the contexts when we want it to.
  • A guard step after the artifact overlay asserts the installed React matches the matrix leg. Without it the dimension is decorative, since the only behavioural difference between the legs is one skipped test, so a 19 leg silently running 18 would pass green.
  • @testing-library/react 14 → 16, plus @testing-library/dom as a direct dependency. Forced, not opportunistic: RTL 14 declares React ^18 only, so the 19 leg cannot run without it. The lockfile shrinks because RTL 14's nested copy collapses into the top-level install; no packages are added.
  • One test is skipped on React 19 only, tracked in Suspense mode resumes with the previous observable's value on React 19 #793.

Notes

Refs #789

@armando-navarro armando-navarro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design here held up everywhere I could push on it. I confirmed main's required status contexts are exactly the four old names, and that v5 requires none, so the suffix scheme is doing real work. The React 19 legs are real.

Two sentences in the commit message came apart when I tested them, and since a squash carries the body into history it may be a good idea to reword them before this merges.

The RTL sentence

"This is forced rather than opportunistic: RTL 14 declares React ^18 only, so the 19 leg cannot run without it." I tried exactly that combination and it runs:

  • npm install --no-save react@19 react-dom@19 on the v5 manifest succeeds under npm 10, with an ERESOLVE warning that it is overriding RTL 14's peer range.
  • The full suite on that tree fails exactly one of its 76 tests: your #793 swap test, same signature.

So the bump is the right call for a different reason: it moves the 19 leg off a combination RTL never declared and npm only runs under protest, onto one RTL 16 supports. That reason deserves to be the one in the history.

The lockfile sentence

"The lockfile shrinks because RTL 14's nested @testing-library/dom collapses into the top-level install." What I found in the lockfiles:

  • @testing-library/dom was already top-level at 9.3.1 on v5, with nothing nested under @testing-library/react.
  • The shrink comes from dom itself moving 9.3.1 to 10.4.1: dom 10 accepts the existing top-level aria-query 5.3.0, so the nested pinned 5.1.3 disappears and takes deep-equal and its tree with it, 11 of the 13 removed entries.
  • "No packages are added" checks out.

The part that would be good to say plainly is that the DOM query engine moved a major, on the 18 legs too. I checked the exposure: the suite queries only by test id, so aria-query's role-computation changes are not exercised. Worth one bullet so the next reader doesn't have to rediscover that.

The database.test.tsx line

One bullet would cover this one:

  • The ReactNode retype appears in neither the What list nor the commit message, and #789 frames this PR as CI-only, so a reader is not scanning test/ for changes.
  • It is forced: reverting it fails the test type-check under RTL 16.
  • It was the only file that still needed it.

Optional, none blocking

  • publish needs the whole test job, so once this reaches main, a red 19 leg skips the canary or release publish even though it gates no merge. There is live precedent: run 31636913522 on main had the Node 24 leg fail and Publish came out skipped. A sentence acknowledging it, or eventually splitting the 19 legs into their own job, whichever you prefer.
  • The install step's if: matrix.react == '19' sits next to the name's matrix.react != '18'. A future non-18 leg would get a suffixed name while running the lockfile's React until your guard fails it red. != '18' on the install makes the two expressions agree.
  • The guard could assert react-dom in the same breath. They install together, so this is belt and braces for a step whose whole point is distrust.
  • The 19 leg runs use-sync-external-store 1.2.0 outside its declared peers (one of the ERESOLVE warnings in the install log names it). Since #793 lists the shim among the suspects and 1.6.0 declares React 19, a one-line note there might save you a false trail. Fine to leave the dep alone given the plan to delete it.

If I've misread what "cannot run" was pointing at, say the word and I'll take another look at that combination.

@jhuleatt

Copy link
Copy Markdown
Collaborator

Does this need to target v5, or could v4 target React 19?

CI type-checks React 19 but has never executed a test against it: the
react matrix exists only on the type-check job, and the test job runs the
lockfile's React 18. This adds react 18/19 as a test-job dimension, so the
suite runs on 19 before any src/ change lands for the React 19 foundation.

@testing-library/react moves 14 -> 16, with @testing-library/dom as a new
direct dependency. This is forced rather than opportunistic: RTL 14 declares
React ^18 only, so the 19 leg cannot run without it. The lockfile shrinks
because RTL 14's nested @testing-library/dom collapses into the top-level
install; no packages are added.

The React 18 legs keep their existing job names, so main's required status
checks keep matching and no admin change is needed. Only the 19 legs get a
suffix, which also means a React 19 failure does not gate a merge while 19
support is still being built.

A guard step after the artifact overlay asserts the installed React matches
the matrix leg. Without it the dimension is decorative: the only behavioural
difference between the legs is one skipped test, so a 19 leg that silently
ran 18 would pass.

That one test is skipped on 19 only, tracked in FirebaseExtended#793: in suspense mode,
swapping the observable resumes with the previous observable's value on
19.2.8 while passing on 18.2.0, deterministic across three runs each.

Emulator jobs go from two to four, which doubles per-PR exposure to the
FirebaseExtended#776 flake.

Refs FirebaseExtended#789
@tyler-reitz
tyler-reitz force-pushed the ci/react-19-test-job branch from 4c5609b to 206b503 Compare August 20, 2026 20:01
@tyler-reitz
tyler-reitz changed the base branch from v5 to main August 20, 2026 20:01
@tyler-reitz

Copy link
Copy Markdown
Contributor Author

It doesn't need v5, and main is the better target: published 4.2.6 declares react: ">=16 || experimental", so npm installs it against React 19 without a warning, while main type-checks React 19 and never runs a test against it. This job closes that gap where it actually exists.

On whether v4 can target React 19, a correction: you asked in our session and I said 19 support was v5-only. Neither branch differs, same peer range, no React 19 work in either.

What differs is the test result. #793 reproduces on shipped 4.2.6, not just v5: 18.2.0 passes 3/3, 19.2.8 fails 3/3, swapping back passes 3/3, same env with only React changed. So v4 permits React 19 without supporting it, and retargeting lands this job with the #793 skip attached, which is the honest state rather than a green light.

Retargeting to main on that basis, since it's CI-only and Armando has already approved. Shout if you'd rather it stayed on v5. The head needed a rebase rather than just a base change, since it was cut from v5 and would otherwise have carried #735 across; the diff is the same five files. The peer-range narrowing stays on v5 either way, since that one is breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants