Skip to content

docs(example): drop the concurrent mode framing from the demo entry - #781

Open
tyler-reitz wants to merge 2 commits into
FirebaseExtended:mainfrom
tyler-reitz:docs/example-concurrent-mode
Open

docs(example): drop the concurrent mode framing from the demo entry#781
tyler-reitz wants to merge 2 commits into
FirebaseExtended:mainfrom
tyler-reitz:docs/example-concurrent-mode

Conversation

@tyler-reitz

Copy link
Copy Markdown
Contributor

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.tsx told readers:

You'll need to use an experimental build of React to use Concurrent mode
https://reactjs.org/docs/concurrent-mode-adoption.html#installation

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/experimental and react-dom/experimental imports existed only to serve that premise, so they go with it.

This removes the last reactjs.org reference 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 calls ReactDOM.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. NonConcurrentModeApp and ConcurrentModeApp are local import aliases in this one file, so renaming them to something like AppWithoutSuspense and AppWithSuspense is 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 withSuspense demo 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 to use() there anyway.

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 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.

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.createRoot is undefined, so the call throws ReactDOM.createRoot is not a function.
  • At build time, with the example's pinned typescript@4.7.4 and @types/react-dom@18.0.6, it fails TS2339: Property 'createRoot' does not exist. createRoot is declared only in client.d.ts, never on the root react-dom entry. Since the example's build is tsc && 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 editing index.tsx. That file still asks for react ^17.0.0, where createRoot does not exist, so the lockfile moves with it. ReactFire itself is fine on 17 (I ran useObservable with suspense: true under legacy ReactDOM.render and 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, since createRoot is not on the root react-dom entry under @types/react-dom@18 either.
  • Worth folding into the same change: the comment says to uncomment the Suspense render but not to drop the existing ReactDOM.render call, so once createRoot does resolve you end up with two roots on one element. On React 18 that warns with You 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.
@tyler-reitz

Copy link
Copy Markdown
Contributor Author

You read it right, nothing to correct.

Caveat added in 8d54e0a, comments only, so your approval should still stand. It now says the Suspense path does not run as checked in, that it needs react and react-dom on 18 or later, and that the ReactDOM.render call has to be replaced rather than left alongside it. I left the commented-out ReactDOM.createRoot block itself untouched on purpose: correcting the specifier there would make the block look runnable, which is the opposite of what the note is doing.

Your three notes are recorded against that follow-up: the bump is example/package.json plus the lockfile rather than just index.tsx, the import has to come from react-dom/client, and uncommenting without deleting the existing render gives two roots on one element. Renames go in with it, agreed, nothing to weigh there.

The useTransition catch is the most useful part. It answers the open question in my last section (whether withSuspense still runs) instead of adding to it, and bounding it to two React 18 APIs under example/ is what makes the follow-up sizeable. Installing the pinned versions to check is what turned my guess into a fact, so thank you for that.

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.

2 participants