docs(example): drop the concurrent mode framing from the demo entry - #781
docs(example): drop the concurrent mode framing from the demo entry#781tyler-reitz wants to merge 2 commits into
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 |
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.