Clean up IndexedDb initialization #2163
Conversation
There was a problem hiding this comment.
LGTM with minor nits.
As an aside, I wouldn't complain if you added a bit more detail in your PR descriptions to make it easier for me to figure out what I'm looking at. 😄 E.g.
- Combined
IndexedDbPersistence.createMultiClientIndexedDbPersistence()andIndexedDbPersistence.createIndexedDbPersistence()factory methods into a single method by relying on the fact that MemorySharedClientState has a suitable "no-op" implementation ofSequenceNumberSyncerthat we can use when multitab is disabled. - Converted new factory method to take an options object for reasons.
Not a huge deal, but with refactors there are often a lot of lines changed (+80,-130 in this case) for a small conceptual change or two. So it's helpful if you can just highlight the conceptual changes and save me the effort of reverse-engineering what changed before I can then re-review and decide if I agree with the changes and whether they were implemented correctly.
| options.lruParams, | ||
| options.queue, | ||
| options.serializer, | ||
| options.sequenceNumberSyncer |
There was a problem hiding this comment.
It seems slightly odd and arbitrary for createIndexedDbPersistence() to take an options object but then we flatten that into separate arguments when calling the IndexedDbPersistence constructor.
I don't care very strongly, but it seems like it'd be cleaner to either convert both to options objects or just use plain arguments for both.
There was a problem hiding this comment.
I agree with "odd", but I wanted to make this long list of arguments more readable, so I opted for the options object for the external API. If I use an options object in the constructor though, then I would have to declare all fields explicitly again. This is my compromise for the best (and worst) of all worlds.
There was a problem hiding this comment.
Oh, right! That's a pretty good reason.
| private readonly queue: AsyncQueue, | ||
| serializer: JsonProtoSerializer, | ||
| lruParams: LruParams, | ||
| private readonly multiClientParams?: MultiClientParams | ||
| private readonly sequenceNumberSyncer?: SequenceNumberSyncer |
There was a problem hiding this comment.
It looks like with this change, you are always passing a sequenceNumberSyncer, so this may not need to be optional?
There was a problem hiding this comment.
For IndexedDb it doesn't have to be optional anymore.
| @@ -278,25 +261,19 @@ export class IndexedDbPersistence implements Persistence { | |||
| // is still experimental. When multi-client is switched to always on, `multiClientParams` will | |||
| // no longer be optional. | |||
| options.lruParams, | ||
| options.queue, | ||
| options.serializer, | ||
| options.sequenceNumberSyncer |
There was a problem hiding this comment.
I agree with "odd", but I wanted to make this long list of arguments more readable, so I opted for the options object for the external API. If I use an options object in the constructor though, then I would have to declare all fields explicitly again. This is my compromise for the best (and worst) of all worlds.
| @@ -278,25 +261,19 @@ export class IndexedDbPersistence implements Persistence { | |||
| // is still experimental. When multi-client is switched to always on, `multiClientParams` will | |||
| // no longer be optional. | |||
| private readonly queue: AsyncQueue, | ||
| serializer: JsonProtoSerializer, | ||
| lruParams: LruParams, | ||
| private readonly multiClientParams?: MultiClientParams | ||
| private readonly sequenceNumberSyncer?: SequenceNumberSyncer |
There was a problem hiding this comment.
For IndexedDb it doesn't have to be optional anymore.
My next PR is going to add one more argument to IndexedDbPersistence, so I thought it was a good idea to clean up the initialization a tiny bit.