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

[tyler-reitz]

Summary

Fixes #383.

• Removes | any from initialData and startWithValue in ReactFireOptions<T>, so passing a value of the wrong type is now a TypeScript error rather than silently accepted
• Tightens checkIdField to read options.idField directly (already typed string | undefined) instead of routing through checkOptions
• Removes checkinitialData and checkOptions (both exported but had zero call sites in the codebase)
• Adds a type cast in useObservable at the initialData assignment to handle the internal mismatch between a hook's user-facing T and the raw Firebase observable type (e.g. DocumentSnapshot<T> vs T)
• Adds casts at rxfire docData/collectionData call sites where rxfire expects keyof T but the field is a plain string — a pre-existing gap in rxfire's types that T | any was silently masking

Breaking changes

Three consumer-visible breaks:

1. initialData / startWithValue type enforcement. Any call site passing an initialData value whose type does not match the hook's T will now get a compile error. Previously T | any collapsed to any and accepted anything silently. For the raw snapshot hooks (useFirestoreDoc, useFirestoreCollection), a DocumentSnapshot<T> previously compiled as initialData and now errors — see Known Limitation below.

2. checkinitialData removal. This function was exported but had zero call sites in the codebase. Any code importing it from 'reactfire' will get a compile error at build time and a module load error at runtime for plain JS importers (ESM import error at load).

3. checkOptions removal. Same situation — exported, zero internal call sites. Compile error for TS consumers, runtime module load error for plain JS importers.

Known limitation

initialData in ReactFireOptions<T> is typed as T, which works correctly for the data hooks (useFirestoreDocData, useFirestoreCollectionData, useDatabaseObjectData, etc.). For the raw snapshot hooks (useFirestoreDoc, useFirestoreDocOnce, useFirestoreCollection), the correct initialData type would be DocumentSnapshot<T> / QuerySnapshot<T>, not T. This mismatch is pre-existing and not introduced here. Tracked in #741 for follow-up.

Test plan

• npm test passes (functions emulator test failures are pre-existing and unrelated)
• npx tsc --noEmit exits clean

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

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

[jhuleatt]

Suggested change

	ReactFire is designed for **web React apps** and wraps the [Firebase JavaScript SDK](https://firebase.google.com/docs/web/setup). It is not compatible with React Native or Expo. For React Native projects, use [react-native-firebase](https://rnfirebase.io/) instead.

[tyler-reitz]

The Platform-support section is already on main, and this rebased PR no longer touches the README. Happy to apply your "Firebase JavaScript SDK" wording, I'll send it as a quick standalone tweak rather than re-adding README to this diff. (Say the word if you'd rather I just fold it in here.)

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