ci: run the test suite against React 19 (#789) - #794
Conversation
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
armando-navarro
left a comment
There was a problem hiding this comment.
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@19on 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/domwas 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-query5.3.0, so the nested pinned 5.1.3 disappears and takesdeep-equaland 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
ReactNoderetype appears in neither the What list nor the commit message, and #789 frames this PR as CI-only, so a reader is not scanningtest/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
publishneeds 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'smatrix.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-domin 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-store1.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.
Why
CI type-checks React 19 but has never executed a test against it: the
reactmatrix 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 anysrc/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
testjob matrix gainsreact: ["18", "19"], so 2 jobs become 4. The 18 legs run the lockfile as-is; the 19 legs installreact@19 react-dom@19 --no-save.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.@testing-library/react14 → 16, plus@testing-library/domas a direct dependency. Forced, not opportunistic: RTL 14 declares React^18only, 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.Notes
peerDependencieschange are deliberately not here; they are a separate review.Refs #789