fix: tighten ReactFireOptions generic types, remove T | any widening

[tyler-reitz]

Summary

Fixes #383. Fixes #741.

• Removes | any from initialData and startWithValue in ReactFireOptions<T>, so a value of the wrong type is a TypeScript error rather than silently accepted
• Types the raw snapshot hooks' options with the hook's own resolved type (types: initialData in ReactFireOptions<T> should accept snapshot types for snapshot hooks #741), so seeding them with a snapshot still compiles
• Tightens checkIdField to read options?.idField directly instead of routing through checkOptions
• Narrows the return type of checkOptions and checkinitialData from any to unknown. Both are kept as exports; removal is v5: remove deprecated and dead exports #754
• Adds casts at the rxfire docData / collectionData call sites, where rxfire expects keyof T and the field is a plain string. A pre-existing gap in rxfire's types that T | any was masking
• Adds src/reactfire-options.type-test.ts, assertions riding the existing tsc --noEmit CI on both React majors, kept out of the tarball by .npmignore

No runtime change.

What breaks

T | any collapses to any, so the generic never enforced anything. Tightening it is what #383 asks for. Three consumer-visible effects:

1. Wrong-typed initialData / startWithValue now errors. This is the point: it catches incorrect code.
2. checkOptions and checkinitialData return unknown instead of any, so a caller using the result has to narrow it.
3. checkIdField returns string | undefined instead of any. Same class, strictly more accurate.

All three are compile-time only. JavaScript consumers are unaffected.

What does not break, thanks to #741

Tightening initialData to T is right for the data hooks and wrong for the raw snapshot hooks, which resolve to a DocumentSnapshot<T> / QuerySnapshot<T>. Left alone it would have broken correct code:

// would have stopped compiling
useFirestoreDoc(ref, { initialData: someDocumentSnapshot });

#741 is fixed here rather than deferred. Three signatures, no runtime change:

useFirestoreDoc<T>(ref, options?: ReactFireOptions<DocumentSnapshot<T>>)
useFirestoreDocOnce<T>(ref, options?: ReactFireOptions<DocumentSnapshot<T>>)
useFirestoreCollection<T>(query, options?: ReactFireOptions<QuerySnapshot<T>>)

Both wrong directions stay rejected: a snapshot hook refuses raw data, a data hook refuses a snapshot.

Why a minor, not a patch

Nothing here breaks correct code, so this does not need the major. But a TypeScript consumer with a wrong initialData will see their build fail, and shipping type changes in a patch is what 4.2.4 did, which is why #749 exists. 4.3.0 is the honest label.

@jhuleatt the semver call is yours. This was briefly retargeted to v5 earlier today, on the basis that it broke correct code; #741 removed that, so it is back on main. If you would rather it ride the major anyway, say so, and the cost to name is that #383 then waits until November.

The type tests fail on a regression

An earlier version of reactfire-options.type-test.ts passed whether or not the tightening was in place: with noUnusedLocals, each unused negative-case const carried its own TS6133, which kept the @ts-expect-error satisfied even when the type error disappeared. Consuming each const fixes it, and that is what is committed. Caught by @armando-navarro.

Mutation-verified both ways:

• reverting initialData?: T to initialData?: T | any fails with TS2578: Unused '@ts-expect-error' directive
• reverting the three snapshot-hook signatures fails the types: initialData in ReactFireOptions<T> should accept snapshot types for snapshot hooks #741 assertions with 2 errors

Test plan

• npm test
• npx tsc --noEmit on both tsconfigs
• Reference docs regenerated with npm run docs:fork

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[armando-navarro]

The tightening looks right to me, and everything I checked came back clean. Two asks below, one decision that belongs to Jeff, and some optional notes.

What I verified

• Packed this branch (merged with main) and type-checked a consumer fixture against it with skipLibCheck: false, on both React 18 and React 19 type packages: wrong-typed initialData and startWithValue are rejected, checkIdField is string | undefined, and every correct usage still compiles.
• The new casts match rxfire 6.1.0's declarations: docData wants keyof R, collectionData wants (U | keyof T) & keyof NonNullable<T>, and database keyField is a plain string, which is why only the firestore call sites need casts.
• Runtime unchanged where it counts: firestore (11/11) and useObservable (14/14) suites pass on the merged result.

Ask 1: commit regenerated reference docs

npm run docs on this branch changes 8 files under docs/reference/, including deleting functions/checkinitialData.md, so the docs workflow will fail. It has not had a chance to yet: a conflicted PR triggers no workflows at all, so the checks list here is empty rather than green, and nothing has run tests or type checks on this PR either.

Ask 2: three description corrections (the squash body becomes the changelog)

• The checkinitialData removal is a runtime break for plain JS importers (ESM import error at load), not TypeScript-only.
• checkOptions' declared return type moves from any to unknown (visible in the emitted dist/index.d.ts); worth listing as a third consumer-visible change.
• For the four snapshot hooks, the T-vs-snapshot mismatch is pre-existing but the enforcement is new: a runtime-correct initialData (a DocumentSnapshot, matching what the hook returns) compiled on main and is now a compile error, while the type demands a plain T that would break a consumer calling .data() on it. Verified against the packed artifact.

Decision for Jeff: semver, and whether #741 rides along

Three consumer-visible breaks under a fix: title, and merges auto-publish. Worth shipping in my view, but how it ships is Jeff's call, same as #735 and #738. Related: should #741 land with this PR so the snapshot hooks never ship demanding the wrong initialData type?

Optional notes

• When you rebase: #731 already restructured the block your useObservable.ts change touches, and your branch type-checks clean on current main with that hunk dropped entirely. Only index.ts and firestore.tsx need to survive.
• checkOptions now has zero call sites in src/, the same rationale used to remove checkinitialData; either say why it stays or remove both in the same breaking release.
• A small type-assertions file with @ts-expect-error on the lines this PR fixes would ride the existing tsc --noEmit CI (both React jobs) as a regression test. Happy to share the fixture I used.
• #738 touches the same idField lines in firestore.tsx; whichever merges second needs another rebase pass.

If I have any of this wrong, say so and I will dig back in. Once the docs and description are in, this is a quick approve from me.

[armando-navarro]

Thanks for turning these around, the description reads accurately now and dropping checkOptions is clean. Approving. Two follow-ups to fold into the rebase pass this needs anyway, neither of which changes the approval, and one of them is my own suggestion coming back to bite.

The type regression test doesn't catch a re-widening

This one is on me for suggesting it without pinning down the mechanics. As written, src/reactfire-options.type-test.ts passes whether or not the tightening is in place, so it would not fail if initialData went back to T | any.

The cause is noUnusedLocals: the negative-case consts are never read, so each line always carries a TS6133 ("declared but its value is never read"). @ts-expect-error is satisfied by any error on the next line, so that unused-local error keeps the directive "used" even when the type error it is meant to catch disappears. I confirmed it locally: reverting initialData?: T to initialData?: T | any leaves tsc --noEmit green.

Consuming each const fixes it, so the type error is the only diagnostic on the line:

// @ts-expect-error initialData must be T, not a different type
const _wrongInitialData: ReactFireOptions<string> = { initialData: 123 };
void _wrongInitialData;

With that, reverting the fix makes the line error-free, the directive goes unused, and tsc fails as intended. One small aside: the file rides in the published tarball via files: ["dist","src"] and is the only test-only file under src/, so a type-tests/ dir or an npmignore would keep it out of consumers' installs.

One regenerated doc file will fail the docs check

docs/reference/classes/ReactFireError.md as committed documents the inherited Error.stackTraceLimit and captureStackTrace statics, but a fresh npm run docs omits them. I checked this against main to be sure it wasn't my setup: a clean npm ci + npm run docs on current main regenerates its committed docs identically here, and both main and this branch pin the same @types/node (25.9.3), so that regen is the canonical one. It looks like this file was generated against an older @types/node. Since docs.yaml regenerates and diffs, it would fail on that file until it is regenerated in a clean environment.

You will hit this naturally on the rebase: the branch is still conflicted, so no substantive CI (Build, tests, the React 18/19 type checks, docs) has run yet, and rebasing onto current main means regenerating the docs again anyway. When you rebase, the useObservable.ts hunk also drops out (superseded by #731).

The semver call stays with Jeff, same as before. Everything else checks out, which is why this is an approve rather than a blocker.

[tyler-reitz]

Thanks Armando, both follow-ups addressed in the rebase:

Type regression test: Added void _wrongInitialData; and void _wrongStartWithValue; after each negative-case const. Verified locally: reverting initialData?: T back to T | any now causes tsc to fail with two unused @ts-expect-error directives as intended.

npmignore: Added .npmignore with src/*.type-test.ts to keep the file out of the published tarball.

Docs: Regen after rebase produced no changes, so the ReactFireError.md statics issue resolved itself on the rebase.

Branch rebased and force-pushed.

[jhuleatt]

I might have missed this elsewhere, but what's the rationale for widening to T | undefined? It looks like this gets cast to ObservableStatus<T> in the return statement

[tyler-reitz]

Good catch, and this is now resolved by a rebase rather than an answer. When I opened this PR its branch predated #732/#750, so the diff was showing a lot of work that has since landed on main independently, including the ObservableStatus<T | undefined> return type. I've rebased onto current main and the widening is no longer part of this PR, it's inherited from main unchanged.

The only firestore change left here is the idField as keyof T cast, which is required because checkIdField now returns a strict string | undefined and rxfire's docData/collectionData expect keyof T.

On the contradiction you spotted (declared T | undefined but cast as ObservableStatus<T>): that is pre-existing on main, so I've left it out of scope here. It's the same honesty gap tracked in #741; I'll fold it into that.

[jhuleatt]

I don't think any of these are actually used, and agree deleting is the right thing, but this is technically a breaking change. Let's leave this for now, since it isn't hurting anything (unless it is, in which case let me know)

Please add an issue reminding us to remove all of these in v5

[tyler-reitz]

Done. Both are kept as exports (no breaking removal), with a comment marking them unused-and-slated-for-v5. Filed #754 to remove them.

One heads-up: tightening initialData to T shifts checkinitialData's inferred return type from any to unknown (visible in its regenerated doc). It's unused internally and on the v5 chopping block, so I left it rather than adding a cast to a dead export, but flagging in case you'd rather I pin it.

[tyler-reitz]

Note it now also covers startWithValue, ClaimsCheck and AuthCheck; startWithValue is not type-only, useObservable reads it as an initialData fallback.

[jhuleatt]

would useInitPerformance be a good option here?

reactfire/src/sdk.tsx

Line 103 in 43dc25a

export const useInitPerformance: InitSdkHook<FirebasePerformance> = (initializer, options) =>

[tyler-reitz]

This was part of the JSX/React-19 migration that has since merged on its own as #732, so the rebase drops performance.tsx from this PR entirely, it's now identical to main.

To answer the question anyway: performance in SuspenseWithPerf is the browser User Timing API (mark/measure), not the Firebase Performance SDK that useInitPerformance initializes, so they aren't interchangeable. That TODO is about a possible future firebase/performance integration.

[jhuleatt]

The change to React.ReactElement happens in a bunch of spots, and could potentially be a breaking change. What are the benefits and risks of switching?

[tyler-reitz]

Same as above, the React.ReactElement move was part of #732 and has already merged, so the rebase drops performance.tsx from this PR (now identical to main).

For the record: the React.ReactElement migration is what made the types React 19 clean (#732). The public return-type change is worth noting for semver, but that decision already shipped with #732.

[jhuleatt]

Why ReactNode vs ReactElement?

[tyler-reitz]

Also from #732, which already merged, so storage.tsx is dropped from the rebased PR (identical to main).

To your point though: the ReactNode (storage) vs ReactElement (auth/sdk/performance) inconsistency is real and worth a small consistency pass on main, independent of this PR.

[jhuleatt]

did TypeScript have trouble inferring this?

[tyler-reitz]

Yes: once initialData is T instead of T | any, initialData ?? startWithValue is T | undefined rather than any, so it no longer flows without a cast. That said, this line is also from the pre-rebase diff, #731 restructured this block on main, so the rebased PR no longer touches useObservable.ts at all.

[tyler-reitz]

Heads-up that I rebased this onto current main and it shrank a lot. When I opened it, the branch predated #732 (JSX/React 19) and #750 (ObservableStatus strict), and it also overlapped #738 (sentinel removal, merged this week), so most of the original diff was already on main. What's left is the actual titled change: initialData/startWithValue tightened to T, the checkIdField rewrite that forces, and a type-level regression test. Several of the comments below resolve themselves as a result. Still a breaking type change, so it needs your semver call, but on a much smaller surface now.

[tyler-reitz]

@jhuleatt before you re-read this: it is much smaller than when you last looked -- now src/index.ts, src/firestore.tsx, a type-test file, .npmignore and regenerated docs.

So four of your threads are stale: performance.tsx and storage.tsx are out of the diff, the useObservable.ts hunk dropped out (superseded by #731), and T | undefined was #733. Both #731 and #733 are already on main.

Two things worth your attention:

• v5: remove deprecated and dead exports #754 grew. You asked for an issue covering checkOptions and checkinitialData; it now also covers startWithValue, ClaimsCheck and AuthCheck. startWithValue is the one to check, since useObservable reads it as an initialData fallback, so removing it deletes a runtime path.
• The description has been rewritten. The old one claimed three breaking changes and none of them were the real one. What actually breaks is now listed plainly, and types: initialData in ReactFireOptions<T> should accept snapshot types for snapshot hooks #741 is fixed here rather than deferred, so nothing that compiles today stops compiling.

[tyler-reitz]

@armando-navarro re-review please, two things changed since your approval.

#741 is folded in. You asked whether it should ride along so the snapshot hooks never ship demanding the wrong initialData type; it does now. Three signatures take ReactFireOptions<DocumentSnapshot<T>> / <QuerySnapshot<T>> instead of the unwrapped type, no runtime change, with type assertions covering both directions. Mutation-checked: reverting the signatures fails them with 2 errors.

#741 stopped being a separate follow-up because deferring it was only ever justified by it looking like design work. It is three signatures.

Also note the removals you approved are back in: Jeff asked to keep checkOptions and checkinitialData for now, and #754 tracks removing them along with startWithValue, ClaimsCheck and AuthCheck.

The semver call is still Jeff's, unchanged.

[tyler-reitz]

Retarget note, since I said the opposite earlier today.

At Monday's sync I said I would route this to v5, on the basis that it broke correct code: the raw snapshot hooks resolve to a DocumentSnapshot/QuerySnapshot, so seeding them with a snapshot stopped compiling, and the fix was a separate issue.

#741 turned out to be three signatures, so it is fixed here and that break is gone. This is back on main as a 4.3.0 candidate.

What remains consumer-visible is ordinary strictness:

• wrong-typed initialData / startWithValue now errors, which is what Feature request: enforce generic types when passed to ReactFireOptions #383 asked for
• checkOptions and checkinitialData return unknown instead of any
• checkIdField returns string | undefined instead of any

That is a minor, not a patch: TS consumers with wrong types will see a build failure, and shipping type changes in a patch is what 4.2.4 did and why #749 exists.

@jhuleatt the semver call is still yours. If you would rather this wait for the major, say so and it goes back to v5; the cost of that is #383 waiting until November.

[armando-navarro]

Thanks Tyler, this is much tighter after the rebase, and the two follow-ups from last time both landed. Re-approving the code.

What I verified

I re-ran the change against a fresh build on the new head:

• tsc --noEmit is clean on both tsconfig.json and tsconfig.test.json, and CI is green on the head.
• The type-test is now load-bearing: reverting the | any widening on initialData and startWithValue fails with four TS2578 unused-directive errors, one per negative case (the two wrong-type assertions and both snapshot-vs-data directions). The void consumption fixed the noUnusedLocals false-pass cleanly.
• checkIdField reading options?.idField directly is behavior-identical to the old checkOptions path, and the docData/collectionData casts are type-only, so the value handed to rxfire is unchanged. The two casts differ only because the two rxfire signatures differ, which checks out against the installed types.
• A clean npm run docs leaves the committed reference docs untouched now, ReactFireError.md included, so the docs check is satisfied.

One thing worth fixing before merge

The .npmignore entry does not actually keep the type-test out of the tarball. npm pack --dry-run on npm 10.9.4 still ships src/reactfire-options.type-test.ts, because files: ["dist","src"] is an allowlist that re-includes everything under src/, and it wins over .npmignore. So the file goes out either way, and both the commit message and the PR summary say it is excluded.

It is cosmetic (a dev-only .ts in the tarball, no runtime effect), but the claim lands in permanent history, so I would either move the type-test into test/ (the type-check job already covers it there, and the tarball does not include test/) or narrow files, and then correct that line. Your call on whether it blocks.

Semver

One thing for the release call: this tightens types, so a consumer whose initialData or startWithValue was the wrong type will now get a build error. Both reads look defensible to me, minor because the code was never actually type-correct and major because an existing build breaks, so I would leave the call to you and Jeff.

If any of this reads wrong, say so and I will take another look.