fix: tighten ReactFireOptions generic types, remove T | any widening#740
fix: tighten ReactFireOptions generic types, remove T | any widening#740tyler-reitz wants to merge 2 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.
Summary
Fixes #383.
Breaking changes
TypeScript-only. Runtime behavior is unchanged.
Known limitation
`initialData` in `ReactFireOptions` 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` / `QuerySnapshot`, not `T`. This mismatch is pre-existing and not introduced here. Tracked in #741 for follow-up.
Test plan