Add toJSON() support for Firestore classes #4298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thebrianchen
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
🦋 Changeset detectedLatest commit: 0a9f5bd The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Changeset File Check
|
Size Analysis ReportAffected Products
|
There was a problem hiding this comment.
I think the code change is fine but I disagree a bit with the direction (but can certainly be convinced). We have our own internal data structures, and we shouldn't make an effort to make the "consumable" by our users. This makes our life more complicated and gives off the impression that we will support the output format going forward. It also convolutes the returned data, with very little additional benefit to the user.
Consider this: What does the user want when they call toJSON on a DocumentReference? They probably would prefer something like:
{
path: 'coll/doc',
firestore: {project: 'foo', database: '(default)'}
converter: null
}
over
{
path: 'coll/doc',
firestore: {project: 'foo', database: '(default)', asyncQueue: {...}, firestoreClient: {...} }
converter: null
}
The first one is more concise, provides the information the user likely needs, and is less likely to change over time.
In case you are wondering where my argument is going (and if it is going anywhere): I think we should just implement toJSON on FirebaseFirestore and call it a day. The toJSON should have enough information to help the developer distinguish it from other instances, that is - database ID and FirebaseApp (and maybe others, but select instance members).
And yes, this should also work for Firbease@exp.
FWIW, the change to FirebaseApp already follows this pattern.
Let me know if you want to talk about this more.
|
@schmidt-sebastian Thanks for the suggestion -- your proposal creates a much more simplified API surface and doesn't require us to maintain a bunch of toJSON() fields. I ended up only including the databaseId and settings, since the project/database fields are not available by default. I'm not sure if I should expand the individual fields or add a return type. WDYT? |
schmidt-sebastian
approved these changes
Jan 19, 2021
Comment on lines
85
to
88
| JSON.stringify(documentSnapshot('foo/bar', { a: 1 }, true)); | ||
| JSON.stringify(documentSnapshot('foo/bar', { a: 1 }, false)); | ||
| JSON.stringify(documentSnapshot('foo/bar', null, true)); | ||
| JSON.stringify(documentSnapshot('foo/bar', null, false)); |
There was a problem hiding this comment.
Do these four cases test different paths in a toJSON call?
| @@ -134,6 +134,13 @@ export class FirebaseFirestore implements FirestoreService { | |||
| return this._terminateTask; | |||
| } | |||
|
|
|||
| toJSON(): object { | |||
| return { | |||
| databaseId: this._databaseId, | |||
There was a problem hiding this comment.
Should we include the app as well? I think the developers might want to see the app name here, since it acts as the main identifier for all Firebase components.
There was a problem hiding this comment.
Added app.name to the JSON.
packages/app-types/index.d.ts
Outdated
| @@ -112,6 +112,8 @@ export interface FirebaseNamespace { | |||
| // Sets log handler for all Firebase components. | |||
| onLog(logCallback: LogCallback, options?: LogOptions): void; | |||
|
|
|||
| toJSON(): object; | |||
There was a problem hiding this comment.
You should also modify the Firestore and the Firebase types if you modify this (firestore-types/index.d.ts and firebase/index.d.ts)
thebrianchen
assigned Feiyang1 and thebrianchen and unassigned thebrianchen and Feiyang1
thebrianchen
force-pushed
the
bc/json-fun
branch
from
71c14c3 to
7c31f91
Compare
Feiyang1
reviewed
Jan 26, 2021
@Feiyang1 Done. Also added support for toJSON() in FirebaseAppLite. |
Feiyang1
reviewed
Jan 26, 2021
Feiyang1
approved these changes
Jan 26, 2021
Co-authored-by: Feiyang <[email protected]>
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4258.
Adding
toJSON()on the Firebase instance breaks the circular reference betweenfirebase.app.containerand the Firestore instance. Similarly, addingtoJSON()to theAsyncQueueandExponentialBackoffallows them to be serialized despite the circular references.Some questions:
app-compat?toJSON()? I'm afraid of locking them in and causing a breaking change if we ever want to change them in the future.