Using Compat-Layer for IndexedDB persistence (v2) #3974
Conversation
|
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis ReportAffected ProductsNo changes between base commit (593e065) and head commit (81b7629). Test Logs
|
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat-v2
branch
from
19836c8 to
c2e7065
Compare
schmidt-sebastian
changed the base branch from
master
to
mrschmidt/treeshakeablefirestoreclient
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat-v2
branch
4 times, most recently
from
7239f49 to
75b105a
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/treeshakeablefirestoreclient
branch
from
d655125 to
6b8f384
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat-v2
branch
from
75b105a to
feb5809
Compare
schmidt-sebastian
changed the title
schmidt-sebastian
force-pushed
the
mrschmidt/enablepersistence-compat-v2
branch
from
feb5809 to
30bbc2f
Compare
| this._receivedInitialUser.promise.then(() => | ||
| this._credentialListener(this._user) | ||
| ); | ||
| _ensureClientConfigured(): FirestoreClient { |
There was a problem hiding this comment.
These all seem like they could be free functions too. Mixing methods and free functions makes development confusing because you have to remember which operations are methods and which are free functions.
If this is painful to implement, beyond the scope of what you want to tackle, or otherwise messy feel free to punt.
There was a problem hiding this comment.
I implemented this. It allows me to merge some of the code from Firestore-Exp and Firestore. I went back and forth on the naming for these functions. The last commit renames them to "ensureFirestoreConfigured" and "configureFirestore".
| if (!this._firestoreClient) { | ||
| this._configureClient(); | ||
| } | ||
| this._firestoreClient!.verifyNotTerminated(); |
There was a problem hiding this comment.
Completely optional: you could avoid the forced unwrap here by having _configureClient return the new instance. Then here you'd have:
let client = this._firestoreClient;
if (!client) {
client = this._configureClient();
}
client.verfyNotTerminated();
return client;This works because initially, client will have the type FirestoreClient | undefined, but because the undefined case results in an unconditional assignment of a FirestoreClient value, tsc will change the inferred type to just FirestoreClient.
There was a problem hiding this comment.
I opted not to do this since this is the only callsite.
| @@ -326,12 +266,12 @@ export function enableMultiTabIndexedDbPersistence( | |||
| * but the client remains usable. | |||
| */ | |||
| function setPersistenceProviders( | |||
| firestore: FirebaseFirestore, | |||
| firestore: FirestoreClient, | |||
There was a problem hiding this comment.
This instance is no longer a Firestore or FirestoreCompat: rename to client or similar?
| // to the components (as `verifyNotInitialized()` would fail). Components can | ||
| // then be accessed via `getOfflineComponentProvider()` and | ||
| // `getOnlineComponentProvider()` | ||
| const firestoreClient = firestore._ensureClientConfigured(); |
There was a problem hiding this comment.
There aren't that many different kinds of clients in this code base, so you can probably abbreviate this to just client if you wanted to. Here and throughout if you choose to.
There was a problem hiding this comment.
Mass-renamed firestoreClient to client.
| settings.host ?? DEFAULT_HOST, | ||
| settings.ssl ?? DEFAULT_SSL, | ||
| /* forceLongPolling= */ false, | ||
| /* forceAutoDetectLongPolling= */ true |
There was a problem hiding this comment.
Why not consult settings for this?
There was a problem hiding this comment.
I originally chose not to pull in FirestoreSettings into firestore-exp since it included a lot of validation code. This is no longer the case so I was able to use FirestoreStettings here.
| } | ||
| enableMultiTabIndexedDbPersistence = enableMultiTabIndexedDbPersistence; |
There was a problem hiding this comment.
This seems like it's a no-op. Did you mean to change a member of this? Here and the next line.
There was a problem hiding this comment.
In this instance, the this member doesn't matter since these new functions should not rely on an associated this context. To make this more readable, I assigned null using bind.
There was a problem hiding this comment.
The thing I'm missing is that this looks like an assignment of a value to itself, which should do nothing and could therefore be removed. What am I missing?
There was a problem hiding this comment.
The left part points to an instance member, while the right part is the free-standing function of the same name. JavaScript can differentiate because access to instance members requires this.
schmidt-sebastian
requested review from
alikn,
andirayo,
avolkovi,
bojeil-google,
ChaoqunCHEN,
davideast,
erikeldridge,
Feiyang1,
hiranya911,
hsubox76,
jamesdaniels,
jsdt,
sam-gc,
yuchenshi,
zijianjoy and
zwu52
as code owners
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
schmidt-sebastian
changed the base branch from
mrschmidt/treeshakeablefirestoreclient
to
master
This is a new version of #3974. The main goal is to keep the code similar to the other SDKs.
Instead of using https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/exp/src/api/components.ts to manage optional dependencies, it transistions firestore-exp to use
FirestoreClient. As a a result, the code fromcomponents.tsis moved to FirestoreClient andfirestore-expcalls into the tree-shakeable FirestoreClient APIs for the first batch of methods.One change from #3974 remains: The credential registration happens in two layers. FirestoreClient registers a listener with FirebaseAuth. This allows us to swap the logic for credential changes when RemoteStore is instantiated (via the OnlineComponentProvider).
Let me know if you want to chat before reviewing.