fix(firestore): restore generic root type exports#8989
fix(firestore): restore generic root type exports#8989russellwheatley wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request introduces generic type support (AppModelType and DbModelType) across the Firestore SDK, including CollectionReference, DocumentReference, DocumentSnapshot, Query, QuerySnapshot, Transaction, and WriteBatch. These changes enable improved type safety when working with custom data models and converters. The review feedback identifies opportunities to simplify type casting in FirestoreTransaction.ts where existing type information makes double-casting redundant.
| new DocumentSnapshot( | ||
| this._firestore, | ||
| data as DocumentSnapshotNativeData, | ||
| (documentRef as unknown as DocumentReference<AppModelType, DbModelType>).converter, |
There was a problem hiding this comment.
The as unknown as cast here is redundant. documentRef is already typed as DocumentReferenceType<AppModelType, DbModelType>, which includes the converter property. Since it has been verified as an instance of DocumentReference on line 81, you can access .converter directly. This is correctly handled without a cast on line 136.
| (documentRef as unknown as DocumentReference<AppModelType, DbModelType>).converter, | |
| documentRef.converter, |
| data as DocumentSnapshotNativeData, | ||
| (documentRef as unknown as DocumentReference<AppModelType, DbModelType>).converter, | ||
| ), | ||
| ) as unknown as DocumentSnapshotType<AppModelType, DbModelType>, |
There was a problem hiding this comment.
The as unknown as cast is likely unnecessary here. A direct cast to DocumentSnapshotType<AppModelType, DbModelType> should be sufficient, as the runtime DocumentSnapshot class is compatible with the DocumentSnapshotType interface declaration.
| ) as unknown as DocumentSnapshotType<AppModelType, DbModelType>, | |
| ) as DocumentSnapshotType<AppModelType, DbModelType>, |
Description
Fix a Firestore TypeScript regression where root imports like
DocumentSnapshot,Transaction, andWriteBatchresolved to non-generic runtime class declarations instead of the intended public generic types.This updates the Firestore root export surface and emitted declarations so root-imported Firestore types behave correctly in type positions again, and adds regression coverage for the reported usage patterns.
Why compare-types did not catch it
compare-typescurrently compares Firestore modular/type files such astypes/firestore.d.tsandmodular.d.ts, not the package rootindex.d.tsconsumer surface.Because the generic types already existed in the modular/type declarations, the script saw the Firestore shapes as matching. The regression was in how the root package entrypoint re-exported those symbols for consumers importing from
@react-native-firebase/firestore, which sits outside the current compare-types boundary.Related issues
closes #8975
Release Summary
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter