docs(example): drop the concurrent mode framing from the demo entry - #781
Conversation
Follow-up to FirebaseExtended#778, which removed the same obsolete premise from the README but left this copy of it. The comment told readers they need "an experimental build of React to use Concurrent mode" and linked reactjs.org/docs/concurrent-mode-adoption.html, a dead page. React 18 shipped in 2022 and concurrent mode was abandoned as a concept rather than stabilised, so the instruction could not be followed. The two commented-out react/experimental and react-dom/experimental imports existed only to serve that premise and go with it. Comments only. No active code changes, so the demo behaves identically. This removes the last reactjs.org reference in the repository. Refs FirebaseExtended#756
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for turning this around so fast, Tyler. Dropping the concurrent-mode framing is the right call, and I confirmed the headline claim: reactjs.org now appears nowhere in the repo, across all tracked files including the generated docs and every lockfile. The change is genuinely comments-only, and the new line about Suspense being off by default and opted into with the suspense prop matches src/firebaseApp.tsx:57. Approving.
One thing I would put on the follow-up pile rather than hold this for.
The uncomment instruction is not runnable yet
The replacement text says to uncomment the import and the render block to see the Suspense version. That block calls ReactDOM.createRoot, and example/package-lock.json pins react-dom at 17.0.2, so following it does not currently get you there. I installed those exact pinned versions to check:
- At runtime
ReactDOM.createRootisundefined, so the call throwsReactDOM.createRoot is not a function. - At build time, with the example's pinned
typescript@4.7.4and@types/react-dom@18.0.6, it failsTS2339: Property 'createRoot' does not exist.createRootis declared only inclient.d.ts, never on the rootreact-domentry. Since the example'sbuildistsc && vite build, that breaks the build too.
The old instruction did not work either, as your description says. I installed react@experimental react-dom@experimental exactly as that comment pointed to, and on the current experimental build createRoot, unstable_createRoot and render are all undefined on the root entry. So this PR is not making anything less runnable. The difference is only that the old line named a precondition and the new one reads as a complete two-step procedure.
Since your deferred item 1 is exactly this, one option is a half-sentence noting the Suspense block needs updating before it runs, so the comment does not promise more than the file can currently deliver. Entirely your call, and I am not attached to my phrasing.
Three small notes on that item while it is in your head.
- Item 1 means bumping
example/package.json, not just editingindex.tsx. That file still asks forreact ^17.0.0, wherecreateRootdoes not exist, so the lockfile moves with it. ReactFire itself is fine on 17 (I ranuseObservablewithsuspense: trueunder legacyReactDOM.renderand the boundary suspends, resolves and keeps updating live), so the bump is the demo's requirement rather than the library's. - If you do move to the modern root API, the import has to come from
react-dom/client, sincecreateRootis not on the rootreact-domentry under@types/react-dom@18either. - Worth folding into the same change: the comment says to uncomment the Suspense render but not to drop the existing
ReactDOM.rendercall, so oncecreateRootdoes resolve you end up with two roots on one element. On React 18 that warns withYou are calling ReactDOMClient.createRoot() on a container that was previously passed to ReactDOM.render(). This is not supported.
On your last section, one concrete input on whether the withSuspense path still runs. It does not on the pinned React, and not only because of createRoot. example/withSuspense/Firestore.tsx:2 imports useTransition, which is also undefined on 17. That one type-checks, since @types/react is already at 18, so it surfaces only at runtime. Those two are the only React 18 APIs anywhere under example/, which at least bounds the React side of the question.
On your item 2, I would just take the renames whenever item 1 lands. AppWithoutSuspense and AppWithSuspense are clearly better and I do not see anything to weigh there.
Separately, the demo cannot be installed as checked out, because example/package.json points at a reactfire-4.0.1.tgz that is not in the repo. That is pre-existing and not yours to fix here, and it is part of why I am treating the above as a note rather than a blocker.
If I have misread any of this, say so and I will take another look.
Armando installed the example's pinned versions and confirmed the uncomment instruction cannot be followed: react-dom is pinned at 17.0.2, where createRoot is undefined at runtime and absent from the root react-dom types, and withSuspense/Firestore.tsx imports useTransition, which is also undefined on 17. Uncommenting the block alongside the existing ReactDOM.render call would also put two roots on one element. The previous wording named a precondition; the replacement read as a complete two-step procedure, which promised more than the file can deliver. This says what is missing instead. Still comments only.
|
You read it right, nothing to correct. Caveat added in Your three notes are recorded against that follow-up: the bump is The |
Three fixes from Armando's review on FirebaseExtended#785. 1. Restore the `set +e` / `set -e` pair around `emulators:exec`. I removed it in 43f39e5 on the reasoning that the script never enables errexit itself. That is true and irrelevant: GitHub runs an undeclared `run:` step as `bash -e {0}`, so errexit is on from the invocation and `set -uo pipefail` does not clear it. The pair was load-bearing. Effect on main today: the first failing iteration kills the step before `rc=$?` is read. Nothing is classified, the arm's tally is never written, and probe-counts.tsv is empty or half written, so the summarize step reports nothing. The probe can only ever produce a clean table, and the first run that genuinely reproduces the flake is the one that reports least. Verified under `bash -e` before and after: without the guard not even the first iteration prints; with it, both arms tally with a failing iteration in each. The comment now says why it exists, because the reasoning that removed it was superficially sound. 2. Scope the flake, hang and health-check searches to firestore's output. This one comes from the widening. In firestore-only mode anything in the log was necessarily about FirebaseExtended#776. In full-suite mode it is not: `expected 'loading' to deeply equal 'success'` is just what vitest prints when a data hook's status assertion fails, and it appears in 6 of the 9 test files. Three separate miscounts followed, all confirmed against logs rather than argued: - a failure in another test file counted as a FirebaseExtended#776 flake - a timeout in any file counted as the FirebaseExtended#776 120s hang - a non-firestore emulator's health check outranked a genuine firestore flake in the same run, filing it as infra and dropping it from the rate entirely The FAIL line names the file and the assertion or timeout sits on the next line, verified against FirebaseExtended#781's real overnight failure, so -A1 is the right window. Captured into a variable rather than piped into `grep -q`, so an early-exit SIGPIPE cannot combine with pipefail and read as a silent no-match. The health check is now firestore-specific, so a non-firestore health failure falls through to the final branch and is recorded in probe-unmatched.txt rather than silently miscounted. That branch also captures the first FAIL line now, not only an assertion, so a genuine failure in another test file is visible too. 3. Raise timeout-minutes 180 -> 240. The 180 was sized on a flat 23s per iteration. Across recent CI runs that step ranges 20 to 31s, and a timeout cares about the slow tail: at 31s the 200 cap wants ~207 minutes. The default 30 per arm is safe under any reading; the ceiling exists for the cap, so it is now set past the pessimistic figure. Comment records the range and the reason. Verified: a 7-log corpus, one real (FirebaseExtended#781's overnight FirebaseExtended#776 failure) and six synthetic, classified under both the old and new logic. The three miscounts above are fixed with no regressions, and the real FirebaseExtended#776 failure still classifies as a flake. YAML parses; zizmor 1.25.2 clean. Refs FirebaseExtended#776, FirebaseExtended#783.
* ci(flake-probe): add a full-suite workload mode The first run of the probe (2026-08-07, run 31204989659) came back 120/120 clean in firestore-only mode with zero RESOURCE_EXHAUSTED. That configuration runs one emulator and one test file, while the #776 failures come from `npm run test`: five emulators and the whole suite. So the isolated suite does not reproduce either failure and cannot serve as a control for the @grpc/grpc-js comparison. Adds a `workload` input, defaulting to `full-suite`, which runs the same command CI runs. `firestore-only` is kept because it isolates the Firestore client and is roughly 3x faster per iteration. Also: - Install the functions deps in full-suite mode. `test.yaml` does this before `npm run test` and the functions emulator does not start without it. - Raise timeout-minutes to 180. Measured per-iteration cost is ~23s full-suite and ~8s firestore-only, so the default 30 per arm is ~28 minutes; the raise is headroom for the 200 cap, which is ~153 minutes full-suite. - Classify a failing `double check - emulator is running` as infra, ahead of the hang check. auth, firestore and database each open with that health check and it fails by timing out, so without this an emulator that never came up was counted as the #776 120s hang. Found by running the real suite on a machine where the RTDB emulator was unreachable, not by inspection. - Make the summary footer workload-aware, so a firestore-only table carries a warning pointing at the 120/120 result. Verified: one full-suite iteration run locally end to end (8 of 9 test files passing, the failure being the unreachable local RTDB emulator), and that real log fed through the classifier, which is what surfaced the health-check misclassification; regression cases for flake, hang, infra and a log where the health check passes alongside a real #776 assertion all classify unchanged; workload input rejects unknown values; zizmor 1.25.2 clean. Refs #776, #783. * ci(flake-probe): fix the errexit abort and scope counting to firestore Three fixes from Armando's review on #785. 1. Restore the `set +e` / `set -e` pair around `emulators:exec`. I removed it in 43f39e5 on the reasoning that the script never enables errexit itself. That is true and irrelevant: GitHub runs an undeclared `run:` step as `bash -e {0}`, so errexit is on from the invocation and `set -uo pipefail` does not clear it. The pair was load-bearing. Effect on main today: the first failing iteration kills the step before `rc=$?` is read. Nothing is classified, the arm's tally is never written, and probe-counts.tsv is empty or half written, so the summarize step reports nothing. The probe can only ever produce a clean table, and the first run that genuinely reproduces the flake is the one that reports least. Verified under `bash -e` before and after: without the guard not even the first iteration prints; with it, both arms tally with a failing iteration in each. The comment now says why it exists, because the reasoning that removed it was superficially sound. 2. Scope the flake, hang and health-check searches to firestore's output. This one comes from the widening. In firestore-only mode anything in the log was necessarily about #776. In full-suite mode it is not: `expected 'loading' to deeply equal 'success'` is just what vitest prints when a data hook's status assertion fails, and it appears in 6 of the 9 test files. Three separate miscounts followed, all confirmed against logs rather than argued: - a failure in another test file counted as a #776 flake - a timeout in any file counted as the #776 120s hang - a non-firestore emulator's health check outranked a genuine firestore flake in the same run, filing it as infra and dropping it from the rate entirely The FAIL line names the file and the assertion or timeout sits on the next line, verified against #781's real overnight failure, so -A1 is the right window. Captured into a variable rather than piped into `grep -q`, so an early-exit SIGPIPE cannot combine with pipefail and read as a silent no-match. The health check is now firestore-specific, so a non-firestore health failure falls through to the final branch and is recorded in probe-unmatched.txt rather than silently miscounted. That branch also captures the first FAIL line now, not only an assertion, so a genuine failure in another test file is visible too. 3. Raise timeout-minutes 180 -> 240. The 180 was sized on a flat 23s per iteration. Across recent CI runs that step ranges 20 to 31s, and a timeout cares about the slow tail: at 31s the 200 cap wants ~207 minutes. The default 30 per arm is safe under any reading; the ceiling exists for the cap, so it is now set past the pessimistic figure. Comment records the range and the reason. Verified: a 7-log corpus, one real (#781's overnight #776 failure) and six synthetic, classified under both the old and new logic. The three miscounts above are fixed with no regressions, and the real #776 failure still classifies as a flake. YAML parses; zizmor 1.25.2 clean. Refs #776, #783.
Follow-up to #778, which removed this same obsolete premise from the README but left this copy of it. Refs #756.
What was wrong
example/index.tsxtold readers:React 18 shipped in 2022 and concurrent mode was abandoned as a concept rather than stabilised, so that instruction cannot be followed and the link is a dead page. The two commented-out
react/experimentalandreact-dom/experimentalimports existed only to serve that premise, so they go with it.This removes the last
reactjs.orgreference in the repository. #778's description originally claimed to have done that, which was wrong, and Armando caught it. This is the correction.Scope
Comments only. No active code changed, so the demo behaves identically. 5 lines added, 6 removed.
I deliberately did not fix two related things I found while in here, because both are more than a comment edit. Pricing them rather than leaving them vague:
1. The example uses the legacy render API, and the commented-out "concurrent" path uses the modern one. The active code calls
ReactDOM.render, deprecated in React 18. The commented-out block callsReactDOM.createRoot, which is now simply the standard API and not experimental at all. So the file currently has it backwards: the path labelled experimental is the current one. Fixing this is a real change to the demo, roughly a 5 line diff plus actually running the example to confirm it still works, and it deserves its own PR rather than riding along in a comment cleanup.2. The identifiers are still named for concurrent mode.
NonConcurrentModeAppandConcurrentModeAppare local import aliases in this one file, so renaming them to something likeAppWithoutSuspenseandAppWithSuspenseis about 3 lines and affects nothing outside it. I left them because this PR touches no active code and I would rather that stay true. Worth doing alongside item 1.Neither is urgent. Both are cheap. Flagging them so they are a decision rather than something that quietly rots for another four years.
Not addressed
Whether the
withSuspensedemo path should be revived, rewritten or deleted is a genuine question and not a docs edit. It has been commented out long enough that nobody knows if it runs. That belongs with the V5 Suspense work, since the suspend path moves touse()there anyway.