[FEAT] Doc/QuerySnapshot fromJSON #8960
Conversation
|
Vertex AI Mock Responses Check
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (1,733,811 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 base branch from
main
to
ddb-firestore-result-serialization
DellaBitta
changed the title
| let curArg = 0; | ||
| let options: SnapshotListenOptions | undefined = undefined; | ||
| if (typeof args[curArg] === 'object' && !isPartialObserver(args[curArg])) { | ||
| console.error('DEDB arg 0 is SnapsotLsitenOptions'); |
There was a problem hiding this comment.
use logError(...) if you want to keep this in the SDK
There was a problem hiding this comment.
This ensures the logging level is correctly applied
There was a problem hiding this comment.
it might even be better to use a hardAssert(...) instead of this if else, to ensure the state is correct. But that will throw.
There was a problem hiding this comment.
This was debug code I left in by accident. Removing it!
| console.error('DEDB arg 0 is SnapsotLsitenOptions'); | ||
| options = args[curArg++] as SnapshotListenOptions; | ||
| } else { | ||
| console.error('DEDB arg 0 is NOT SnapsotLsitenOptions'); |
There was a problem hiding this comment.
use logError(...) if you want to keep this in the SDK
| >( | ||
| db: Firestore, | ||
| json: object, | ||
| ...args: unknown[] |
There was a problem hiding this comment.
seems like you could just change this to converter?: FirestoreDataConverter<AppModelType, DbModelType>. It would improve readability of the code since there are no other overloads where this could be another type.
| >( | ||
| db: Firestore, | ||
| json: object, | ||
| ...args: unknown[] |
| static fromJSON< | ||
| NewAppModelType = DocumentData, | ||
| NewDbModelType extends DocumentData = DocumentData | ||
| >( | ||
| firestore: Firestore, | ||
| json: object, | ||
| converter?: FirestoreDataConverter<NewAppModelType, NewDbModelType> | ||
| ...args: unknown[] |
There was a problem hiding this comment.
same suggestion. Just to be clear, I'm not recommending removing the overloads, just renaming the param in the implementation
There was a problem hiding this comment.
Done! Thanks for the tip.
| */ | ||
| private readJsonString(length: number): string { | ||
| if (this.cursor + length > this.bundleData.length) { | ||
| throw new Error('Reached the end of bundle when more is expected.'); |
There was a problem hiding this comment.
It may be more appropriate to throw FirestoreError
| bundleSource: 'QuerySnapshot', | ||
| bundleName: 'test name' | ||
| }); | ||
| }).to.throw; |
There was a problem hiding this comment.
IMO, it's usually good to assert that the error message is like expected. Mostly because bad or incorrect error messages can make for bad DX. Just a comment for your consideration.
DellaBitta
merged commit c88ea8e
into
ddb-firestore-result-serialization
Discussion
This is a PR pointed at a feature branch.
Add a methods to parse bundles synchronously to extract single
DocumentSnapshotsorQuerySnapshotscontaining documents. Hook the new bundle parser up thedocumentSnapshotFromJSONandquerySnapshotFromJSON.Testing
Local testing with Next.js app.
Added unit tests and integration tests.
API Changes
go/fs-js-json-serialization