Skip to content

demo(recipe): swap the hand-rolled hooks for ReactFire - #798

Open
tyler-reitz wants to merge 6 commits into
recipe-demofrom
recipe-demo-framework
Open

demo(recipe): swap the hand-rolled hooks for ReactFire#798
tyler-reitz wants to merge 6 commits into
recipe-demofrom
recipe-demo-framework

Conversation

@tyler-reitz

@tyler-reitz tyler-reitz commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Swaps the vanilla app's hand-rolled Firestore and auth plumbing for ReactFire. The diff against recipe-demo is the deliverable: same app, same behavior, nothing added.

Not a merge candidate. This branch and #797 exist to be read, not landed.

What ReactFire replaces

Deleted Replaced by
src/lib/use-recipes.ts, 44 lines of hydrate-then-subscribe useFirestoreCollectionData(query, { idField, initialData })
src/lib/session-context.tsx, 18 lines of context over onAuthStateChanged useUser() and useSigninCheck(), called directly

+43 / -93 across nine files under src/, plus package.json and its lockfile. src/lib/providers.tsx (16 lines) is new. The homepage still fetches on the server with the Firebase JS SDK, not the Admin SDK, and passes the result in as initialData: the list is public, so no identity is needed and the rules stay in play.

Findings

It does not build against published reactfire. SessionNav calls useUser from the root layout, so every prerendered route hits a hook: Missing getServerSnapshot, which is required for server-rendered content. Patched build renders 8 <article> server-side, published 4.2.6 fails the build, patched restored renders 8 again. #779 is a prerequisite, not an improvement.

initialData is per-hook, and the server list fits only one query. Passing the unfiltered list to every cuisine query painted all 12 recipes as that cuisine's result until the snapshot landed, and made status: 'loading' unreachable, so RecipeList's aria-busy branch was dead here and live on vanilla. Selecting Japanese, before and after the fix:

immediately aria-busy settled
before 12 articles, all five cuisines absent 3 Japanese
after 1 article present 3 Japanese

The fix is the finding: the guard must be the key's absence, since useObservable tests hasOwnProperty('initialData') (src/useObservable.ts:78). initialData: undefined reports success with no data, typechecks clean, throws at runtime.

Also lost, none of it fixed here
Subscriptions leak per filter Each cuisine's listener outlives switching away: an abandoned Japanese subject still received a write with nobody viewing it. The vanilla hook unsubscribed on every filter change. This is #790, user-visible
Errors take the whole page useObservable re-throws unconditionally, so the inline error branch was deleted. With no route-level error.tsx the throw reaches Next's top-level boundary and replaces the document including the nav. Vanilla named the failure in place. The cuisine filter needs a composite index neither branch ships, so a real project's first filter click is the likely trigger. #735 fixes the re-throw, v5-only
createdAt changes type under one declared type Declared string, and the deleted client hook ran toRecipes on every snapshot to hold that. toRecipes survives and still normalises the server fetch, which is why the seeded render carries a string, but rxfire returns raw data, so the seeded render carries a string and live snapshots a Timestamp, and data as Recipe[] asserts it away. A correctness bug, not a typing nicety. withConverter is the fix and is out of scope here
The server payload is discarded after first mount force-dynamic re-runs the fetch on every soft navigation back, but the cached observable already has a value so the overlay is skipped. Confirmed with a sentinel that never appeared. The deleted hook repainted on every remount
Hydration hazard, forced not observed useUser seeds from auth.currentUser, null on the server. A session restoring before hydration renders the email where the server rendered a loading row. SessionNav is in the root layout, so if it fires it is every route
The client bundle grows 15% ReactFire brings rxjs, rxfire and the use-sync-external-store shim with it. Production builds of both branches, all .next/static JavaScript: 1,288,940 to 1,487,391 bytes raw (+198 KB), 387,966 to 446,189 gzipped (+58 KB). Both arms built clean and produced different totals, so the measurement discriminates
Three invisible changes <FirestoreProvider> has no consumer (nothing calls useFirestore()); rxfire's user() wraps onIdTokenChanged not onAuthStateChanged, so token refresh re-renders; useUser and useSigninCheck register separately, so /create-recipe holds two auth subscriptions where the context held one

Two pages are byte-identical to vanilla, which is also a result: signin has no wrapper, and create-recipe has no AI Logic binding. RequireAuth grows 29 lines to 30, since useSigninCheck still returns a status.

The dependency

reactfire@4.2.6-exp.ac3ccf9, the per-commit build published from main at ac3ccf9. Pinned exactly: npm i writes a caret, and a clean resolve of that range picks published 4.2.6, since a release outranks a prerelease. An existing lockfile masks it, so the first install looks correct. A file:.. link was rejected because npm symlinks the repo root in, bringing a second React.

Verification

Emulators unless noted, controls in both directions.

Check Result
Server render 8 <article>, negative control on an absent title
Cuisine filter, real project 3 Japanese, aria-busy reachable, unpatched arm as control
Published 4.2.6 Build fails, Missing getServerSnapshot, exit 1
Caret vs exact pin Clean resolve gives 4.2.6; exact pin gives the exp build
Like, live subscription Like (0) to Liked (1) with no reload; an external write arrived unprompted
Route protection Redirects to /signin?next=%2Fcreate-recipe; ?next=https://example.com/pwned lands on /
Console, tsc Clean, both confirmed able to fail first

Not verified: /create-recipe generation on this branch. Needs a real project with AI Logic enabled.

The filter, subscription, createdAt, force-dynamic and hydration findings came from Armando Navarro's review of this PR.

Replaces the app's own Firestore and auth plumbing with ReactFire hooks,
so the diff against recipe-demo is the comparison itself.

- src/lib/use-recipes.ts (42 lines) becomes useFirestoreCollectionData
  with initialData, keeping the server-fetched list as the seed
- src/lib/session-context.tsx (17 lines) is deleted; useUser and
  useSigninCheck are called from the components that need them
- src/lib/providers.tsx wires FirebaseAppProvider, FirestoreProvider and
  AuthProvider to the existing module-level instances

The dependency is a packed build of main at ac3ccf9, committed under
recipe-demo/ with a gitignore exception. Published 4.2.6 predates #779,
and without it this app does not build: SessionNav calls useUser from the
root layout, so prerendering /_not-found and /signin fails with "Missing
getServerSnapshot". Measured both ways, with the patched build restored
afterwards.

RecipeBrowser loses its error branch because useObservable on main
re-throws rather than returning status 'error', and the recipe list needs
a cast because useFirestoreCollectionData's generic requires a typed
Query. Both are findings for the PR description rather than problems to
work around here.
# Conflicts:
#	recipe-demo/src/lib/use-recipes.ts

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

What I want to raise is a theme rather than a list of defects: the comparison currently undersells what adopting ReactFire costs here. You disclose the error-handling regression, and I found four more behaviours that changed without a mention. None of it blocks anything, since the branch is not going anywhere, but each is something a reader of the diff would want to know.

What the ReactFire side loses, that the diff does not show

The cuisine filter shows the wrong recipes

initialData: initialRecipes is the server's unfiltered list, and it is handed to every filter rather than only the one it belongs to.

  • useObservable's overlay forces status: 'success' for it, so selecting a cuisine paints the whole unfiltered list, labelled as that cuisine's result, until the real snapshot lands.
  • loading={status === 'loading'} can therefore never be true, so RecipeList's aria-busy branch is unreachable here and live on the vanilla side.
  • The guard against this is in the file the PR deletes. use-recipes.ts at d4d1af1, the swap commit's own parent, carries renderedCuisine and if (cuisine !== renderedCuisine.current) setStatus('loading').
  • What worked when I tried it: omit the key on other filters, cuisine === 'all' ? { idField: 'id', initialData: initialRecipes } : { idField: 'id' }. Four lines for four, so your +42 / -94 is unchanged.
  • The shorter initialData: cuisine === 'all' ? initialRecipes : undefined does not work, which I found by trying it first. useObservable tests hasOwnProperty('initialData'), so the key being present is what counts, not its value.
  • That form therefore reports success with data undefined, which is a TypeError in RecipeList and typechecks clean.

createdAt changes type under the same declared type

Recipe.createdAt is declared string, and toRecipes used to normalise it on every snapshot.

  • rxfire returns raw document data, so the seeded render carries a string and live snapshots carry a Firestore Timestamp. I saw both in one session.
  • data as Recipe[] asserts that away, and nothing renders the field today, which is what makes it easy to inherit.
  • The first new Date(recipe.createdAt) a reader writes behaves differently depending on which render it lands in.
  • Your withConverter aside is the fix. I would frame it as a correctness fix rather than a typing nicety.

Filter subscriptions are never released

Each cuisine opens a listener that outlives the switch away from it.

  • I subscribed to Japanese, switched to Mexican, then wrote a Japanese recipe while nobody was viewing that filter.
  • The abandoned subject went from two documents to three, so it was still live and still receiving server updates.
  • The vanilla hook returned onSnapshot's unsubscribe from its effect, so every filter change tore the previous listener down.

There is an ordering where hydration mismatches

I could only produce it by forcing that order, so treat this as a hazard rather than something I saw happen on its own. useUser seeds from auth.currentUser only when it is truthy, and on the server that is always null.

  • The server HTML is <li aria-busy="true"><span>Loading</span></li>.
  • If the browser's session restores before hydration, the client takes the seeded branch and renders the email instead.
  • Forcing that order gave me React's "Hydration failed because the server rendered text didn't match the client", settling on the email.
  • The deleted SessionProvider could not do this. It always started at { user: null, status: 'loading' } and only moved inside an effect, which never runs before hydration.
  • I could not show the race happens on its own. Auth restores persistence from IndexedDB asynchronously, so currentUser is normally still null when React hydrates.
  • Your Console row is itself evidence it does not fire in ordinary use, and I am not disputing that row.
  • Raising it because SessionNav is in the root layout, so if it does fire it is every route, and RecipeCard's disabled={!user} is exposed the same way.

After the first mount, the server payload is fetched and thrown away

page.tsx is force-dynamic, so every soft navigation back to / re-runs the fetch and ships a fresh initialRecipes.

  • The cached observable already has a value, so useObservable skips the overlay and never reads the payload.
  • I checked with a sentinel payload and it never appeared on the return visit.
  • The deleted hook used useState(initialRecipes), so it painted the server list on every remount.

On the error section

Your re-throw reasoning is right, and deleting the branch rather than leaving it dead was the correct call. One sentence I would change, and one thing I would not do.

  • "Unmounts to the nearest boundary" understates it. There is no route-level error.tsx, so the throw escapes past the root layout to Next's built-in top-level boundary, which replaces the whole document including the nav.
  • The vanilla branch kept the page up and named the failure inline, which is the comparison worth stating.
  • I would not add an error.tsx to fix it, because that puts code on one side only and moves the diff the deliverable rests on.
  • On my reading of how Next nests the segment boundary it would not catch SessionNav anyway, since the root layout renders that above the route's own boundary.
  • Worth pairing with it: the cuisine filter needs a composite index that neither branch ships, so on a real project the first cuisine selection is the likely trigger, and it now takes the page rather than rendering inline.

The committed dependency

This repo publishes an exp-tagged build per commit to main, and 4.2.6-exp.ac3ccf9 is on npm from exactly the commit you packed. Its src/ is byte-identical to the tarball's and the demo builds against it, so the binary and its supporting machinery could all go.

  • Pin it exactly. npm i reactfire@4.2.6-exp.ac3ccf9 writes ^4.2.6-exp.ac3ccf9, and that caret matches the published 4.2.6, which is the release you correctly say cannot build this app.
  • A clean resolve picks 4.2.6, which has no getServerSnapshot at all, so the demo lands back on the failure your Findings section leads with.
  • "reactfire": "4.2.6-exp.ac3ccf9" with no caret resolves correctly.
  • The regeneration recipe does not reproduce the file. package.json at ac3ccf9 is version 4.2.6, so npm pack emits reactfire-4.2.6.tgz, and the rename is undocumented.
  • The vendored copy declares itself 4.2.6, so npm ls reactfire cannot tell it from the published release.
  • Its dist/index.js also carries build-path comments from the machine that packed it (//#region ../../../../../../home/me/reactfire/...), which the npm build does not. Nothing runtime, but it means the bytes cannot be reproduced from this repo.

Smaller

  • <FirestoreProvider sdk={firestore}> has no consumer here: useFirestoreCollectionData takes its instance from the query it is handed, and nothing calls useFirestore().

  • I removed the provider and its imports and the app still compiled, so a reader counting three providers is counting one that does nothing here.

  • The swap changes which auth listener runs. rxfire's user() wraps onIdTokenChanged where authState() wraps onAuthStateChanged, so components re-render on token refresh rather than only on sign-in and sign-out.

  • useUser and useSigninCheck register under different observable ids, so /create-recipe holds two auth subscriptions where the deleted context held one and fanned it out.

  • RequireAuth grew rather than shrank: 8 added, 7 deleted, 29 lines to 30. Small, but it sits in the paragraph making the line-count argument, where everything around it is exact.

If I have misread any of these, particularly the hydration ordering or the filter behaviour, tell me which and I will run it again.

The server list is unfiltered, so handing it to every cuisine query painted
all recipes as that cuisine's result until the snapshot landed, and made
status 'loading' unreachable so RecipeList's aria-busy branch was dead on
this side and live on vanilla.

initialData is passed only for the 'all' query. It has to be the key's
absence rather than an undefined value: useObservable tests
hasOwnProperty('initialData'), so initialData: undefined reports success
with no data, which typechecks clean and throws at runtime.

Found by Armando Navarro in review of #798.
reactfire publishes an exp build per commit to main, and 4.2.6-exp.ac3ccf9
is the same commit the vendored tarball was packed from, so the binary and
its .gitignore exception are unnecessary.

Pinned exactly. The caret npm writes by default resolves to published 4.2.6
on a clean install, which has no getServerSnapshot and cannot build this app.
An existing lockfile hides that, so the first install looks correct.

Found by Armando Navarro in review of #798.
@tyler-reitz

Copy link
Copy Markdown
Contributor Author

Fixed the filter one and took the exp-tag swap. The rest is disclosed in the body rather than fixed, since the branch is a comparison artifact.

Reproduced it before fixing, selecting Japanese against a real project:

immediately aria-busy settled
before 12 articles, all five cuisines absent 3 Japanese
after 1 article present 3 Japanese

Took your four-line form. The mechanism is config.hasOwnProperty('initialData') at src/useObservable.ts:78, and the shorter form typechecks clean, which is what makes it silent rather than a compile error.

Your caret warning reproduces, with one condition worth adding: an existing lockfile masks it. npm i reactfire@4.2.6-exp.ac3ccf9 writes the caret but locks the exp build, so the first install looks right, and only a clean resolve picks 4.2.6. Pinned exactly, tarball gone, headline re-checked: published 4.2.6 still fails the build, the exp build succeeds.

The abandoned subscriptions are #790 rather than a demo bug, and your walk-through is the clearest live evidence of it so far. withConverter stays out, since it would widen the diff the comparison rests on, so the createdAt drift is disclosed instead. Hydration goes in as a hazard with your caveat attached.

Also confirmed: useFirestore() has no consumers here, so the provider count is one higher than it looks.

@tyler-reitz
tyler-reitz requested a review from jhuleatt August 24, 2026 20:41
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