Ensure persistence started #1043
Conversation
| @@ -96,7 +96,10 @@ export class IndexedDbPersistence implements Persistence { | |||
| static MAIN_DATABASE = 'main'; | |||
|
|
|||
| private simpleDb: SimpleDb; | |||
| private started: boolean; | |||
| private hasStarted: boolean; | |||
There was a problem hiding this comment.
The general convention for exposing properties that are only readonly via the public API is to use an underscore. So this would be:
private _started: boolean;
get started(): boolean {
return this._started;
}
Please also move the getter to where the other getters/methods are defined. You could also initialize _started with 'false'.
| @@ -140,12 +142,15 @@ export class IndexedDbPersistence implements Persistence { | |||
| .then(() => { | |||
| this.scheduleOwnerLeaseRefreshes(); | |||
| this.attachWindowUnloadHook(); | |||
| }) | |||
| .then(() => { | |||
| this.hasStarted = true; | |||
There was a problem hiding this comment.
This can be part of the previous then block.
But that being said, I don't this will work one I merge my multi-tab PR. In the new scheduleOwnerLeaseRefreshes I am using started to determine whether I should issue callbacks. We essentially need three states: not-started, starting, started. And since nothing else should be happening while we call .start(), it should be safe to keep flipping the started bit in the beginning?
There was a problem hiding this comment.
As discussed, leaving this at the end in its own .then() call.
| @@ -163,6 +163,10 @@ export class LocalStore { | |||
| */ | |||
| private garbageCollector: GarbageCollector | |||
| ) { | |||
| assert( | |||
| persistence.started, | |||
| 'Local Store passed an unstarted persistence implementation' | |||
There was a problem hiding this comment.
The LocalStore is the receiver here, which doesn't quite come across in this message (maybe it is just missing a was as in was passed )
| @@ -45,18 +45,21 @@ export class MemoryPersistence implements Persistence { | |||
| private remoteDocumentCache = new MemoryRemoteDocumentCache(); | |||
| private queryCache = new MemoryQueryCache(); | |||
|
|
|||
| private started = false; | |||
| private hasStarted = false; | |||
There was a problem hiding this comment.
Same comment about naming.
| @@ -75,6 +75,11 @@ export interface Persistence { | |||
| */ | |||
| start(): Promise<void>; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Nit: I would move the property declarations to the top.
| this.persistence = new Promise(resolve => { | ||
| const p = this.getPersistence(this.serializer); | ||
| p.start().then(() => resolve(p)); | ||
| }); |
There was a problem hiding this comment.
Hmm, this is somewhat strange. Can we not start the persistence layer in the renamed start()? We can check there whether it is already started using your new nifty API.
There was a problem hiding this comment.
We technically could, but doRestart() calls start(), and doRestart() intentionally does not want to start the persistence layer. As such, I think it makes more sense to keep the call to persistence.start() in the one place where we know we want to start it, rather than gate it with an if (!persistence.started) await persistence.start(), which then means you have to know why it would or wouldn't already be started.
| async shutdown(): Promise<void> { | ||
| await this.remoteStore.shutdown(); | ||
| await this.persistence.shutdown(/* deleteData= */ true); | ||
| await (await this.persistence).shutdown(/* deleteData= */ true); |
There was a problem hiding this comment.
Please try to find a way to make this a non-Promise again.
There was a problem hiding this comment.
Restructured to have an init and a start.
This mostly fixes up the spec tests so that we can assert that
LocalStorealways gets an already-startedPersistenceinstance.