[Feat] Firestore onSnapshot support for bundles for feature branch. #8896
Conversation
Vertex AI Mock Responses Check
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (92,564 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
|
DellaBitta
changed the title
MarkDuckworth
left a comment
MarkDuckworth
left a comment
There was a problem hiding this comment.
Looks good. I think the only thing that needs to be fixed is checking for unsubscribed before creating a snapshot listener for a DocumentReference
| ) => void, | ||
| error: args[curArg++] as (error: FirestoreError) => void, | ||
| complete: args[curArg++] as () => void | ||
| }, |
There was a problem hiding this comment.
nit: it feels like you could reduce some duplicate code by creating these observer objects higher in the function. It may only reduce two calls to onSnapshot[Query|Document]SnapshotBundle
There was a problem hiding this comment.
Yeah, that's what I had originally but it ended up being more lines of code due to the type definiton and then configuring the object with optional parameters. I can revist it though, if you'd like.
Edit: revisited, and done!
| * | ||
| * @internal | ||
| */ | ||
| function normalizeSnapshotJsonFields(snapshotJson: object): { |
There was a problem hiding this comment.
I might have used a type predicate for this. However, since you are also checking values, this implementation is also okay, IMO.
DellaBitta
merged commit e34353d
into
ddb-firestore-result-serialization
3 participants
Discussion
Pull request to add onSnapshot listeners support for bundles to the Firestore SSR serialization feature branch.
This change adds a series of onSnapshot overloads that match the existing onSnapshot variants (observer, individual callback functions, etc) but accepts a bundle instead of a QuerySnapshot or a DocumentSnapshot.
The toJSON methods of
QuerySnapshotandDocumentSnapshothave also been updated to explicitly name the fields in the object return type.Fixed an issue in the bundle builder where the bundleName for
DocumentSnapshotsdidn't match theResourcePathof the document, which is needed when reconstituting theDocumentReferenceinonSnapshot.Finally, I cleaned up the text wrapping for the
onSnapshotreference doc comments so they take up fewer lines of source code.Testing
Added integration tests to ensure that
onSnapshotfunctions invoked with a bundle properly callnextwith the snapshot data initially and when the document is updated in the service, anderrorif the bundle is invalid. Made tests that exercise the method signatures that take an Observer as well.API Changes
Numerous
onSnapshotoverloads that now accept a bundle a parameter. These will be part of a formal API proposal once the full feature branch implementation is closer to completion.