Skip to content

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

Open
tyler-reitz wants to merge 4 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/reactfire-options-generic-types
Open

fix: tighten ReactFireOptions generic types, remove T | any widening#740
tyler-reitz wants to merge 4 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/reactfire-options-generic-types

Conversation

@tyler-reitz

@tyler-reitz tyler-reitz commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

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

Removes the `| any` escape hatch from `initialData` and `startWithValue`
in ReactFireOptions<T>, so passing a mismatched type is now a compile
error instead of silently accepted.

Also tightens the return types of `checkIdField` and `checkinitialData`,
and adds a cast in `useObservable` to handle the internal mismatch
between the hook's T and the raw Firebase observable type.

Closes FirebaseExtended#383
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

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

checkIdField now reads options.idField directly (already string | undefined)
instead of routing through checkOptions and casting. checkinitialData had
no call sites and is removed.

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

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

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.

Feature request: enforce generic types when passed to ReactFireOptions

2 participants