Refactor some FirestoreClient methods #3507
Conversation
💥 No ChangesetLatest commit: b6b38db Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
schmidt-sebastian
force-pushed
the
mrschmidt/smallerdiff
branch
from
4883762 to
cde5600
Compare
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
requested review from
alikn,
andirayo,
avolkovi,
bojeil-google,
davideast,
erikeldridge,
Feiyang1,
hiranya911,
hsubox76,
jamesdaniels,
jsdt,
sam-gc,
scottcrossen,
zijianjoy and
zwu52
as code owners
schmidt-sebastian
changed the base branch from
mrschmidt/treeshakeablefc
to
master
schmidt-sebastian
force-pushed
the
mrschmidt/smallerdiff
branch
from
b247ffd to
5cfb7b8
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/smallerdiff
branch
from
5cfb7b8 to
4bb0930
Compare
| @@ -65,7 +63,7 @@ export function getDoc<T>( | |||
| const ref = cast<DocumentReference<T>>(reference, DocumentReference); | |||
| const firestore = cast<Firestore>(ref.firestore, Firestore); | |||
| return getFirestoreClient(firestore).then(async firestoreClient => { | |||
| const viewSnapshot = await getDocViaSnapshotListener( | |||
| const viewSnapshot = await firestoreClient.getDocumentFromLocalCache( | |||
There was a problem hiding this comment.
Why does this use getDocumentFromLocalCache and not getDocumentViaSnapshotListener like the other refactorings in this file?
There was a problem hiding this comment.
This is a bug. Thanks for catching.
| @@ -415,6 +418,20 @@ export class FirestoreClient { | |||
| ); | |||
| } | |||
|
|
|||
| async getDocumentViaSnapshotListener( | |||
There was a problem hiding this comment.
Optional: consider something like getSingleDocumentViaSnapshotListener. As it stands, these function names are really similar to each other.
There was a problem hiding this comment.
This is actually the name that was used previously and matches other SDKs, such as Android. I am leaving as is for consistency.
The next PR is going to have a larger diff so this one extracts the change out that I made for two methods. I moved this methods to
firestore_client.tsto match the other methods of #3492.In the follow up PR, I will then drop FirestoreClient from firestore-exp and will directly call the new methods.