LocalStore-only queries #3508
LocalStore-only queries #3508
Conversation
💥 No ChangesetLatest commit: e519317 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/fromcache-v2
branch
from
76b41a7 to
f650ee0
Compare
Binary Size ReportAffected SDKs
Test Logs |
| @@ -111,7 +111,6 @@ export class Firestore | |||
| * Only ever called once. | |||
| */ | |||
| protected _terminate(): Promise<void> { | |||
| debugAssert(!this._terminated, 'Cannot invoke _terminate() more than once'); | |||
There was a problem hiding this comment.
This doesn't work anymore as this is not called from the AsyncQueue (by the overridden method)
schmidt-sebastian
force-pushed
the
mrschmidt/smallerdiff
branch
2 times, most recently
from
5cfb7b8 to
4bb0930
Compare
| const viewSnapshot = await firestoreClient.getDocumentFromLocalCache( | ||
| firestoreClient, | ||
| return getEventManager(firestore).then(eventManager => | ||
| enqueueReadDocumentViaSnapshotListener( |
There was a problem hiding this comment.
Question: why is this instance of firestoreClient.getDocumentFromLocalCache replaced with enqueueReadDocumentViaSnapshotListener, while the one below is replaced by enqueueReadDocumentFromCache?
There was a problem hiding this comment.
This is getDoc, which does a "default" read (from Server first but with the option of reading from cache if offline). The one below is getDocFromCache, which only reads from cache.
| verifyNotInitialized(firestoreImpl); | ||
| const settings = firestoreImpl._getSettings(); | ||
|
|
||
| // `_getSettings()` freezes the client settings and prevents further changes |
There was a problem hiding this comment.
Nit: should this comment be before the call to _getSettings, for consistency with the previous one in this file?
| firestoreImpl._queue, | ||
| remoteStore, | ||
| persistence, | ||
| /* enabled=*/ true |
There was a problem hiding this comment.
Ultranit: there is a space after the opening token but not before the closing token -- make this consistent?
There was a problem hiding this comment.
:) Fixed here and below.
| firestore | ||
| ); | ||
| if (onlineComponentProviderPromise) { | ||
| logDebug(LOG_TAG, 'Removing OnlineComponentProvider'); |
There was a problem hiding this comment.
Question: nothing is logged if the promise was not found -- is this intentional?
There was a problem hiding this comment.
Yeah, I think that is ok. We only log when components are initialized and when they are terminated.
This removes FirestoreClient from the firestore-exp build. FirestoreClient depends on all components in the system, and as such it is not very well geared for a tree-shakeable SDK.
Instead, the firestore-exp SDK now purely relies on Component Providers and splits up offline-only and online use. This allows LocalStore-only queries in getDocFromCache and getDocsFromCache, dropping their size from 220Kb to 130Kb (ish).
Couple things:
doc()) is pretty impossible though and hence I don't think this affects users bundle sizes.