Compat class for DocumentSnapshot #4051
Conversation
🦋 Changeset detectedLatest commit: 6f8e9a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
schmidt-sebastian
force-pushed
the
mrschmidt/documentsnapshot-compat-v2
branch
from
b9d5c40 to
30c25d2
Compare
| @@ -233,14 +237,15 @@ export function getDocsFromCache<T>( | |||
| ): Promise<QuerySnapshot<T>> { | |||
| const firestore = cast(query.firestore, FirebaseFirestore); | |||
| const client = ensureFirestoreConfigured(firestore); | |||
| const userDataWriter = newExpUserDataWriter(firestore, query._converter); | |||
There was a problem hiding this comment.
Optional: you could save a little bit of memory by deferring the creation of the userDataWriter until you need it just as you're creating the QuerySnapshot.
There was a problem hiding this comment.
I slightly prefer the current arrangement as I can use the same pattern across all functions. It also keeps the assignment in the Lambda shorter, which makes it fit one in line.
| @@ -1233,3 +1247,14 @@ export function newUserDataReader( | |||
| serializer | |||
| ); | |||
| } | |||
|
|
|||
| export function newExpUserDataWriter<T>( | |||
There was a problem hiding this comment.
I'm surprised this doesn't live closer to the UserDataWriter. It doesn't seem like it belongs in reference.ts.
Also, it's confusing that this is named newExpUserDataWriter but it lives in the lite SDK?
There was a problem hiding this comment.
Huh, you do catch all the weirdness here :) I was hoping I could get away with this weird name. I ended up fixing this by applying your suggestion below. There is now a LiteUserDataWriter, an ExpUserDataWriter and an UserDataWriter (which we could name ClassicUserDataWriter, but I don't yet use this term in our class names). Note that LiteUserDataWriter and ExpUserDataWriter are essentially the same, but I kept them separate for clarity. I also re-exported the DocumentReference types from /exp/src/api/reference for additional clarity.
| firestore: FirebaseFirestore, | ||
| converter: UntypedFirestoreDataConverter<T> | null | ||
| ): UserDataWriter { | ||
| return new UserDataWriter( |
There was a problem hiding this comment.
A more traditional arrangement would be to have UserDataWriter be an abstract class and have different subclasses supply the conversion routines by implementing methods that are missing. Then you wouldn't need a newExpUserDataWriter function, you'd just invoke the constructor of the subtype instead.
There was a problem hiding this comment.
I updated the PR as suggested (which would match what we discussed last week). I was a bit lazy in the first iteration, as the factory functions were already implemented.
| convertValue(value: ProtoValue): unknown { | ||
| convertValue( | ||
| value: ProtoValue, | ||
| serverTimestampBehavior: ServerTimestampBehavior = 'none' |
There was a problem hiding this comment.
Why can't this remain a constructor parameter? That seemed like a cleaner arrangement.
There was a problem hiding this comment.
Since I am now "injecting" the UserDataWriter when the DocumentSnapshot is constructed, I don't yet know what the setting for serverTimestampBehavior will be.
|
|
||
| protected convertReference(name: string): DocumentReference { | ||
| const key = this.convertDocumentKey(name, this.firestore._databaseId); | ||
| return DocumentReference.forKey(key, this.firestore, /* converter= */ null); |
There was a problem hiding this comment.
This seems like it now unconditionally creates a reference without a converter, where previously the converter was propagated in from the API construct that was used to create the reference/snapshot. Why is this OK?
There was a problem hiding this comment.
I thought about this problem, but I think this new behavior actually makes sense. We don't know anything about the document that is referenced here, so it seems like we shouldn't assume that the original converter applies here. I think this is an edge case that probably no one noticed, but it would be pretty challenging to continue exposing the old behavior if we deemed this a breaking change.
There was a problem hiding this comment.
I think it's reasonable to call the old behavior a bug and call this a fix :-).
We should probably call this out in the release notes.
|
|
||
| protected convertReference(name: string): DocumentReference { | ||
| const key = this.convertDocumentKey(name, this.firestore._databaseId); | ||
| return DocumentReference.forKey(key, this.firestore, /* converter= */ null); |
There was a problem hiding this comment.
I think it's reasonable to call the old behavior a bug and call this a fix :-).
We should probably call this out in the release notes.
This is the Compat class for DocumentSnapshot. The only real challenge here is that it we now either generate firestore-exp or Classic DocumentReferences or Bytes/Blob types. To facilitate this, I pass the UserDataWriter as an argument to the DocumentSnapshot.