Add ComponentProvider #2828
Add ComponentProvider #2828
Conversation
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/nomultitab
branch
2 times, most recently
from
2610229 to
46354ac
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/nomultitab
branch
from
47903d8 to
bddc340
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/nomultitab
branch
from
bddc340 to
9fb5da1
Compare
| * Initializes and wires up all core components for Firestore. Consumers have to | ||
| * invoke initialize() once before accessing any of the individual components. | ||
| */ | ||
| export abstract class ComponentProvider { |
There was a problem hiding this comment.
Do you think this would end up as less code if this were just an interface that exposed the properties?
There was a problem hiding this comment.
I changed this into an abstract class to share the asserts. I don't feel strongly.
| datastore: Datastore, | ||
| clientId: ClientId, | ||
| initialUser: User, | ||
| maxConcurrentLimboResolutions: number, |
There was a problem hiding this comment.
This seems like something that could be handled in a test-only subclass.
Indeed, it would be more flexible if instead of requiring subclasses to implement initialize, you required them to implement the getFoo calls and remember the result. Then for any dependency one component needs call getBar for it.
The downside of this approach would be that it's less deterministic, but the upside would be that:
- the core initialization logic could be completely shared
- test configurations that were just slightly different could be created by overriding just the one method's factory method.
- You could pull in the loadConnection and Datastore construction here as well; it's slightly oddly outside here now
WDYT?
There was a problem hiding this comment.
I tried to use shared code here, especially since this is likely the reason why the code size went up for the combined build. It's pretty tricky though since the providers instantiate different types and the IndexedDb implementation assign some listeners that are only available with multi tab (e.g. the primary state listener). If I want to use shared code I have to undo at least some of the state encapsulation that I tried to achieve.
As for removing initialize(): I debated doing this as well, but it doesn't change much in the code structure. getSyncEngine() would actually have to initialize LocalStore, which means that getSyncEngine() now has to take all the state required by both LocalStore and SyncEngine. On top of that, it would now be async.
We can probably remove some overhead by removing the asserts and exposing the member variables as properties. That would also remove the need for the abstract class.
There was a problem hiding this comment.
I played around with trying to make this work and I agree this ends up looking pretty wonky. This is a snippet of what I had:
private _persistenceKey?: string;
getPersistenceKey(): string {
if (this._persistenceKey == null) {
this._persistenceKey = IndexedDbPersistence.buildStoragePrefix(
this._databaseInfo
);
}
return this._persistenceKey;
}
private _sharedClientState?: SharedClientState;
getSharedClientState(): SharedClientState {
if (this._sharedClientState == null) {
this._sharedClientState = this._persistenceSettings.synchronizeTabs
? new WebStorageSharedClientState(
this._asyncQueue,
this._platform,
this.getPersistenceKey(),
this._clientId,
this._initialUser
)
: new MemorySharedClientState();
}
return this._sharedClientState;
}
private _persistence?: Persistence;
async getPersistence(): Promise<Persistence> {
if (this._persistence == null) {
this._persistence = await IndexedDbPersistence.createIndexedDbPersistence(
{
allowTabSynchronization: this._persistenceSettings.synchronizeTabs,
persistenceKey: this.getPersistenceKey(),
clientId: this._clientId,
platform: this._platform,
queue: this._asyncQueue,
serializer: this.getSerializer(),
lruParams: LruParams.withCacheSize(this._persistenceSettings.cacheSizeBytes),
sequenceNumberSyncer: this.getSharedClientState()
}
);
}
return this._persistence;
}I think this side-steps the fundamental problem of needing to undo the state encapsulation you did for multi-tab because each implementation keeps variables of the types it wants and getFoo() methods can use a covariant return type.
In this implementation there's still an initialize() method, but essentially all it does is assign fields and then build the components map by invoking the getter for each entry in the components map. Any internal cross-wiring happens as a side-effect of these creation functions.
The trouble seems to be that while there is some shared code, only a few of them are truly copy/paste. That, in turn, doesn't seem to justify the considerable additional complexity of making the on-demand wiring work. I think it can be made to work, but I also think it's pretty expensive in terms of code and therefore may not be worth it.
I think the case gets better for this structure as you pull more construction in, including platform.loadConnection and constructing Datastore. However, that can also share that code between implementations of ComponentProvider by simpler, more ad-hoc means.
I've sent it back for your consideration in case you disagree with the conclusion I'm drawing from the experiment. If you like the idea, let's run with it. If you're not enthusiastic, let's skip it.
There was a problem hiding this comment.
I cleaned up 312e4cf a bit and chose that as my favorite. The reason I favor this are:
- It creates a simple initialization flow that makes the sequence of events clear (or at least as clear as it might ever be).
- It removes the need for async getters. If we want to return started components, almost all getters would need to be async (even WebStorageSharedClientState has a start()).
- It ports better than
e7cb1de.
Note that in the example above, we would even have to factor out WebStorageSharedClientState since we don't want to pull this into the Memory-only build (it isn't part of it right now).
I also updated the PR a bit: I moved the initialization of Datastore from the Auth-Token handler to initializeComponents (and added a TODO to make this part of the ComponentProvider if we ever figure out a clean strategy). As a consequence, we may now initialize Datastore, WebChannelConnection and Serializer more than once since we would re-run this code during persistence fallback. The construction of these objects seems pretty simple though, and hence I am not too worried about the overhead:
| @@ -238,7 +240,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { | |||
| targetId, | |||
| status === 'current' | |||
| ); | |||
| if (this.isPrimary) { | |||
| if (this.isPrimaryClient) { | |||
There was a problem hiding this comment.
It seems like this factoring leaves a lot of multi-tab knowledge in the base SyncEngine. Looking at how this logic breaks down I can't say I have any great ideas for how to improve that though.
|
This seems like it could be two PRs: one that implements componentization, which seems unambiguously like a good idea that could be ported to the other platforms, and the multi-tab split which brings up separate issues (but also seems like a good idea). |
|
I removed the Multi-Tab changes from this PR and pushed two different version: 312e4cf uses properties and removes the asserts. It's not necessarily following proper TypeScript conventions though (since the properties don't exist before e7cb1de is a type safe version that returns a list of initialized components. It is not very pretty though and might not port well. It does remove the size increase of this PR though. I can pull in Datastore once we come up with a final strategy :) |
There was a problem hiding this comment.
I don't have a strong preference for either 312e4cf or e7cb1de. The former seems more likely to be directly portable, and it's a pattern we already use (e.g. in the FirestoreClient). I don't feel super strongly though, so if you have a preference, either is ultimately fine with me. Take a look at the notes I have on splitting up initialize before choosing though.
| datastore: Datastore, | ||
| clientId: ClientId, | ||
| initialUser: User, | ||
| maxConcurrentLimboResolutions: number, |
There was a problem hiding this comment.
I played around with trying to make this work and I agree this ends up looking pretty wonky. This is a snippet of what I had:
private _persistenceKey?: string;
getPersistenceKey(): string {
if (this._persistenceKey == null) {
this._persistenceKey = IndexedDbPersistence.buildStoragePrefix(
this._databaseInfo
);
}
return this._persistenceKey;
}
private _sharedClientState?: SharedClientState;
getSharedClientState(): SharedClientState {
if (this._sharedClientState == null) {
this._sharedClientState = this._persistenceSettings.synchronizeTabs
? new WebStorageSharedClientState(
this._asyncQueue,
this._platform,
this.getPersistenceKey(),
this._clientId,
this._initialUser
)
: new MemorySharedClientState();
}
return this._sharedClientState;
}
private _persistence?: Persistence;
async getPersistence(): Promise<Persistence> {
if (this._persistence == null) {
this._persistence = await IndexedDbPersistence.createIndexedDbPersistence(
{
allowTabSynchronization: this._persistenceSettings.synchronizeTabs,
persistenceKey: this.getPersistenceKey(),
clientId: this._clientId,
platform: this._platform,
queue: this._asyncQueue,
serializer: this.getSerializer(),
lruParams: LruParams.withCacheSize(this._persistenceSettings.cacheSizeBytes),
sequenceNumberSyncer: this.getSharedClientState()
}
);
}
return this._persistence;
}I think this side-steps the fundamental problem of needing to undo the state encapsulation you did for multi-tab because each implementation keeps variables of the types it wants and getFoo() methods can use a covariant return type.
In this implementation there's still an initialize() method, but essentially all it does is assign fields and then build the components map by invoking the getter for each entry in the components map. Any internal cross-wiring happens as a side-effect of these creation functions.
The trouble seems to be that while there is some shared code, only a few of them are truly copy/paste. That, in turn, doesn't seem to justify the considerable additional complexity of making the on-demand wiring work. I think it can be made to work, but I also think it's pretty expensive in terms of code and therefore may not be worth it.
I think the case gets better for this structure as you pull more construction in, including platform.loadConnection and constructing Datastore. However, that can also share that code between implementations of ComponentProvider by simpler, more ad-hoc means.
I've sent it back for your consideration in case you disagree with the conclusion I'm drawing from the experiment. If you like the idea, let's run with it. If you're not enthusiastic, let's skip it.
| components.sharedClientState.onlineStateHandler = onlineState => | ||
| components.syncEngine!.applyOnlineStateChange( | ||
| this.sharedClientState.onlineStateHandler = onlineState => | ||
| this.syncEngine!.applyOnlineStateChange( |
There was a problem hiding this comment.
this.syncEngine is already declared as force unwrapped. Is the ! necessary?
This PR removes most of the size overhead of Multi-Tab from the memory only build and may even allow us to create a slimmer non-Multi-Tab IndexedDB build at some point.
It does this by creating a subclass of LocalStore and SyncEngine and upgrading the PersistenceProvider to a ComponentProvider that is used by both FirestoreClient and the Spec tests to initialize and wire up a lot of the core components.
The PR also removes all non-supported multi-tab only operations from MemoryPersistence.