Don't use interface types #3770
Conversation
💥 No ChangesetLatest commit: dba59c3 Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Binary Size ReportAffected SDKs
Test Logs |
| const query = cast<Query<T>>(ref, Query); | ||
| firestore = cast(query.firestore, Firestore); | ||
| internalQuery = query._query; | ||
| firestore = cast(ref.firestore, FirebaseFirestore); |
There was a problem hiding this comment.
nit: s/ref/reference (here and throughout) since you're already standardizing it elsewhere.
| extends LiteDocumentSnapshot<T> | ||
| implements firestore.DocumentSnapshot<T> { | ||
| private readonly _firestoreImpl: Firestore; | ||
| export interface FirestoreDataConverter<T> { |
There was a problem hiding this comment.
Is there any reason why you're defining these types here instead of importing them from the firestore main SDK? For example, I don't follow why we import SnapshotMetadata from firestore/src/api/database.ts in this file, but not SnapshotOptions. Is there something special about SnapshotMetadata?
There was a problem hiding this comment.
In our code, SnapshotMetadata is an actual class, which makes it a bit different from the other types that are merely defined in the interface. I just cross-checked some types and could not find any other type that is already defined in the sources.
| @@ -19,7 +19,7 @@ import { FirebaseApp as FirebaseAppLegacy } from '@firebase/app-types'; | |||
| import { FirebaseApp as FirebaseAppExp } from '@firebase/app-types-exp'; | |||
| import { deleteApp } from '@firebase/app-exp'; | |||
| import * as legacy from '@firebase/firestore-types'; | |||
| import * as exp from '../../exp-types'; | |||
| import * as exp from '../index'; | |||
There was a problem hiding this comment.
Just to make sure I'm following the big picture here: Is the ultimate goal for no files to depend on any of the exports in exp-types/index.d.ts?
Also, what is rough plan to use the existing instance of FirebaseFirestore instead of instantiating a new one?
There was a problem hiding this comment.
The goal of this entire effort is to remove the explicit types, which will allow us to write JSDoc comments in code and use the same JSDoc documentation across all implementations. That does mean that the typings file will go away in its current form. The generated form will not be used internally and only shipped to customers.
| import { removeComponents } from './components'; | ||
|
|
||
| declare module '@firebase/component' { |
There was a problem hiding this comment.
Can you please explain what this module here is doing?
There was a problem hiding this comment.
This is not my area of expertise... this piece of code extends the available services that we can theoretically fetch via getComponent to include firestore/lite. This is needed to avoid a compile time error in this file (at the bottom, we have getFirestore() for the Lite SDK, which is compiled but not used when building firestore-exp).
This PR updates the firestore-exp and firestore/lite SDK so that we no longer have to maintain manual type definitions.
The first commit:
The second commit removes the now unnecessary
cast()calls from the interface to the SDK type from most callsites. I maintained the cast to FirebaseFirestore in some instances as this allows me to share DocumentReference, CollectionReference and Query between the different SDKs (the firestore-exp SDK used the Lite classes, which are only typed to return LiteFirestore and not ExpFirebaseFirestore).The third commit regenerates the dependency files.
There should be no logic changes.