fix: tighten ReactFireOptions generic types, remove T | any widening#740
fix: tighten ReactFireOptions generic types, remove T | any widening#740tyler-reitz wants to merge 4 commits into
Conversation
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
|
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
left a comment
There was a problem hiding this comment.
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-typedinitialDataandstartWithValueare rejected,checkIdFieldisstring | undefined, and every correct usage still compiles. - The new casts match rxfire 6.1.0's declarations:
docDatawantskeyof R,collectionDatawants(U | keyof T) & keyof NonNullable<T>, and databasekeyFieldis a plainstring, 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
checkinitialDataremoval is a runtime break for plain JS importers (ESM import error at load), not TypeScript-only. checkOptions' declared return type moves fromanytounknown(visible in the emitteddist/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-correctinitialData(aDocumentSnapshot, matching what the hook returns) compiled on main and is now a compile error, while the type demands a plainTthat 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.tschange touches, and your branch type-checks clean on current main with that hunk dropped entirely. Onlyindex.tsandfirestore.tsxneed to survive. checkOptionsnow has zero call sites insrc/, the same rationale used to removecheckinitialData; either say why it stays or remove both in the same breaking release.- A small type-assertions file with
@ts-expect-erroron the lines this PR fixes would ride the existingtsc --noEmitCI (both React jobs) as a regression test. Happy to share the fixture I used. - #738 touches the same
idFieldlines infirestore.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
left a comment
There was a problem hiding this comment.
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.
Summary
Fixes #383.
| anyfrominitialDataandstartWithValueinReactFireOptions<T>, so passing a value of the wrong type is now a TypeScript error rather than silently acceptedcheckIdFieldto readoptions.idFielddirectly (already typedstring | undefined) instead of routing throughcheckOptionscheckinitialDataandcheckOptions(both exported but had zero call sites in the codebase)useObservableat theinitialDataassignment to handle the internal mismatch between a hook's user-facingTand the raw Firebase observable type (e.g.DocumentSnapshot<T>vsT)docData/collectionDatacall sites where rxfire expectskeyof Tbut the field is a plainstring— a pre-existing gap in rxfire's types thatT | anywas silently maskingBreaking changes
Three consumer-visible breaks:
initialData/startWithValuetype enforcement. Any call site passing aninitialDatavalue whose type does not match the hook'sTwill now get a compile error. PreviouslyT | anycollapsed toanyand accepted anything silently. For the raw snapshot hooks (useFirestoreDoc,useFirestoreCollection), aDocumentSnapshot<T>previously compiled asinitialDataand now errors — see Known Limitation below.checkinitialDataremoval. 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).checkOptionsremoval. Same situation — exported, zero internal call sites. Compile error for TS consumers, runtime module load error for plain JS importers.Known limitation
initialDatainReactFireOptions<T>is typed asT, which works correctly for the data hooks (useFirestoreDocData,useFirestoreCollectionData,useDatabaseObjectData, etc.). For the raw snapshot hooks (useFirestoreDoc,useFirestoreDocOnce,useFirestoreCollection), the correctinitialDatatype would beDocumentSnapshot<T>/QuerySnapshot<T>, notT. This mismatch is pre-existing and not introduced here. Tracked in #741 for follow-up.Test plan
npm testpasses (functions emulator test failures are pre-existing and unrelated)npx tsc --noEmitexits clean