fix!: type makeDocumentSnapshot as DocumentSnapshot - #337
Conversation
v14 removed the legacy namespaced API from the root firebase-admin entry point, so `firestore(...)` was undefined and makeDocumentSnapshot threw. Move to the modular subpaths (getFirestore, DocumentReference, GeoPoint, Timestamp from firebase-admin/firestore; App, deleteApp, initializeApp, AppOptions from firebase-admin/app). The DocumentReference branch of objectToValueProto read a `_referencePath` internal that no longer exists, emitting `projects//databases//<path>`. It now reads projectId and databaseId off the Firestore instance and includes the missing `documents` segment. The old spec assertion could not catch this because DocumentReference.toString() returned "[object Object]" on the bundled @google-cloud/firestore. Drops admin ^8 and ^9 from peerDependencies. The modular subpaths have no exports entry before v10, and this package has already imported firebase-admin/firestore since v3.
The firebase-admin devDependency stays at ^12 because firebase-functions ^4.9.0 peer-requires ^10 || ^11 || ^12, so CI does not exercise v14 yet.
The `projectId` getter throws until the client has resolved a project, which happens when a Firestore instance is created before the test SDK sets GCLOUD_PROJECT. Read the field and fall back to the env instead. Also adds the changelog entries for this PR.
`snapshot_` is absent from the public Firestore typings, so the service variable was left implicitly `any` and the declaration file shipped `any` as the return type. Type the service and cast the `snapshot_` call. The v2 created and deleted event mocks declare a QueryDocumentSnapshot, so the snapshot they build is cast to it.
There was a problem hiding this comment.
Code Review
This pull request improves type safety for Firestore mocks by explicitly typing makeDocumentSnapshot to return DocumentSnapshot instead of any, introducing a helper type FirestoreWithSnapshotBuilder to handle the internal snapshot_ method, and updating tests and cloud event mocks accordingly. Feedback suggests typing the project variable as string | undefined rather than string in makeDocumentSnapshot to ensure type safety when strictNullChecks is enabled, as the project ID can be undefined.
CorieW
left a comment
There was a problem hiding this comment.
LGTM
Note: This is a breaking change. I've edited your PR title to reflect this.
ajperel
left a comment
There was a problem hiding this comment.
I think we could maybe help reduce the number of places where folks need to cast as a result of this change dramatically if we do some function overloads for typing makeDocumentSnapshot return value. Something like:
/**
* Create a DocumentSnapshot for a non-existent document.
*/
export function makeDocumentSnapshot(
/** Pass in `{}` to mock the snapshot of a document that doesn't exist. */
data: Record<string, never>,
/** Full path of the reference (e.g. 'users/alovelace') */
refPath: string,
options?: DocumentSnapshotOptions
): DocumentSnapshot;
/**
* Create a QueryDocumentSnapshot populated with document data.
*/
export function makeDocumentSnapshot(
/** Key-value pairs representing data in the document. */
data: { [key: string]: any },
/** Full path of the reference (e.g. 'users/alovelace') */
refPath: string,
options?: DocumentSnapshotOptions
): QueryDocumentSnapshot;
/** Implementation */
export function makeDocumentSnapshot(
data: { [key: string]: any },
refPath: string,
options?: DocumentSnapshotOptions
): DocumentSnapshot {
let firestoreService: Firestore;
let project: string | undefined; // Note: projectId can be undefined when GCLOUD_PROJECT is unset
...
What do you think?
There was a problem hiding this comment.
Not strictly part of this change. But I think this could be return type QueryDocumentSnapshot since it always has data right?
There was a problem hiding this comment.
Yes your exactly right, QueryDocumentSnapshot makes much more sense here
…e-document-snapshot # Conflicts: # CHANGELOG.md # spec/providers/firestore.spec.ts # src/providers/firestore.ts
|
@ajperel I think this is a great idea and would improve the casting/typing experience a lot here. I'll add the overloads along with type tests for both cases, so {} gives a DocumentSnapshot and data gives a QueryDocumentSnapshot. |
Overloads return QueryDocumentSnapshot when data is passed and DocumentSnapshot for {}, matching what snapshot_ builds at runtime. exampleDocumentSnapshot always has data, so it returns QueryDocumentSnapshot too.
| - chore: drop support for Node 18 and below (minimum supported version is now Node 20) | ||
| - fix: support firebase-admin v14 by moving to the modular `firebase-admin/app` and `firebase-admin/firestore` entry points (#327) | ||
| - breaking: drop firebase-admin `^8` and `^9` from the peer dependency range | ||
| - fix: type `makeDocumentSnapshot` as returning a `QueryDocumentSnapshot` when given data and a `DocumentSnapshot` when given `{}`, rather than `any` (#327) |
There was a problem hiding this comment.
Don't we also need to note this as "breaking" even though the break cases are also rare.
There was a problem hiding this comment.
yes will update this now
Covers the typing request in #327.
makeDocumentSnapshotwas typed asany. It now returnsQueryDocumentSnapshotwhen given data andDocumentSnapshotwhen given{}, which matches what Firestore'ssnapshot_builds at runtime.exampleDocumentSnapshotreturnsQueryDocumentSnapshot, since it always has data.snapshot_isn't in the public Firestore typings, so the call goes through a local type rather than leakinganyinto the shipped.d.ts.QueryDocumentSnapshot, so the snapshot they build is cast to it. No runtime change.anymay need adjusting, but passing data or{}directly now gets the right type without a cast.Type tests cover both overloads and
exampleDocumentSnapshot, and fail if the return types widen or swap.