From a857f59de52dffac022f6279e8cff3a36355b4c6 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 17 Jul 2020 16:08:23 -0700 Subject: [PATCH 1/3] Remove MultiTabLocalStore --- .../firestore/src/core/component_provider.ts | 17 +- .../firestore/src/core/firestore_client.ts | 6 +- packages/firestore/src/core/sync_engine.ts | 60 +--- packages/firestore/src/local/local_store.ts | 274 ++++++++---------- .../firestore/src/local/memory_persistence.ts | 4 + packages/firestore/src/local/persistence.ts | 8 + packages/firestore/src/remote/datastore.ts | 2 + .../test/unit/local/local_store.test.ts | 10 +- .../test/unit/specs/spec_test_runner.ts | 6 +- 9 files changed, 170 insertions(+), 217 deletions(-) diff --git a/packages/firestore/src/core/component_provider.ts b/packages/firestore/src/core/component_provider.ts index 100bf23308a..66a76b7ea68 100644 --- a/packages/firestore/src/core/component_provider.ts +++ b/packages/firestore/src/core/component_provider.ts @@ -23,9 +23,8 @@ import { } from '../local/shared_client_state'; import { LocalStore, - MultiTabLocalStore, newLocalStore, - newMultiTabLocalStore + synchronizeLastDocumentChangeReadTime } from '../local/local_store'; import { MultiTabSyncEngine, @@ -133,7 +132,6 @@ export class MemoryComponentProvider implements ComponentProvider { ); this.remoteStore.syncEngine = this.syncEngine; - await this.localStore.start(); await this.sharedClientState.start(); await this.remoteStore.start(); @@ -298,7 +296,6 @@ export class IndexedDbComponentProvider extends MemoryComponentProvider { * `synchronizeTabs` will be enabled. */ export class MultiTabIndexedDbComponentProvider extends IndexedDbComponentProvider { - localStore!: MultiTabLocalStore; syncEngine!: MultiTabSyncEngine; async initialize(cfg: ComponentConfiguration): Promise { @@ -318,14 +315,12 @@ export class MultiTabIndexedDbComponentProvider extends IndexedDbComponentProvid } } }); - } - createLocalStore(cfg: ComponentConfiguration): LocalStore { - return newMultiTabLocalStore( - this.persistence, - new IndexFreeQueryEngine(), - cfg.initialUser - ); + // In multi-tab mode, we need to read the last document change marker from + // persistence once during client initialization. The next call to + // `getNewDocumentChanges()` will then only read changes that were persisted + // since client startup. + await synchronizeLastDocumentChangeReadTime(this.localStore); } createSyncEngine(cfg: ComponentConfiguration): SyncEngine { diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 48819f5bca0..3ec55b5eb9d 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -202,7 +202,8 @@ export class FirestoreClient { enableNetwork(): Promise { this.verifyNotTerminated(); return this.asyncQueue.enqueue(() => { - return this.syncEngine.enableNetwork(); + this.persistence.setNetworkEnabled(true); + return this.remoteStore.enableNetwork(); }); } @@ -334,7 +335,8 @@ export class FirestoreClient { disableNetwork(): Promise { this.verifyNotTerminated(); return this.asyncQueue.enqueue(() => { - return this.syncEngine.disableNetwork(); + this.persistence.setNetworkEnabled(false); + return this.remoteStore.disableNetwork(); }); } diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index 7d9dd1fda86..0937b77d321 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -17,9 +17,13 @@ import { User } from '../auth/user'; import { + getNewDocumentChanges, + getCachedTarget, ignoreIfPrimaryLeaseLoss, LocalStore, - MultiTabLocalStore + getCurrentlyActiveClients, + lookupMutationDocuments, + removeCachedMutationBatchMetadata } from '../local/local_store'; import { LocalViewChanges } from '../local/local_view_changes'; import { ReferenceSet } from '../local/reference_set'; @@ -225,10 +229,6 @@ export interface SyncEngine extends RemoteSyncer { handleCredentialChange(user: User): Promise; - enableNetwork(): Promise; - - disableNetwork(): Promise; - getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet; } @@ -940,14 +940,6 @@ class SyncEngineImpl implements SyncEngine { } } - enableNetwork(): Promise { - return this.remoteStore.enableNetwork(); - } - - disableNetwork(): Promise { - return this.remoteStore.disableNetwork(); - } - getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet { const limboResolution = this.activeLimboResolutionsByTarget.get(targetId); if (limboResolution && limboResolution.receivedDocument) { @@ -1016,38 +1008,10 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { // `isPrimary` is true. private _isPrimaryClient: undefined | boolean = undefined; - constructor( - protected localStore: MultiTabLocalStore, - remoteStore: RemoteStore, - datastore: Datastore, - sharedClientState: SharedClientState, - currentUser: User, - maxConcurrentLimboResolutions: number - ) { - super( - localStore, - remoteStore, - datastore, - sharedClientState, - currentUser, - maxConcurrentLimboResolutions - ); - } - get isPrimaryClient(): boolean { return this._isPrimaryClient === true; } - enableNetwork(): Promise { - this.localStore.setNetworkEnabled(true); - return super.enableNetwork(); - } - - disableNetwork(): Promise { - this.localStore.setNetworkEnabled(false); - return super.disableNetwork(); - } - /** * Reconcile the list of synced documents in an existing view with those * from persistence. @@ -1097,7 +1061,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { error?: FirestoreError ): Promise { this.assertSubscribed('applyBatchState()'); - const documents = await this.localStore.lookupMutationDocuments(batchId); + const documents = await lookupMutationDocuments(this.localStore, batchId); if (documents === null) { // A throttled tab may not have seen the mutation before it was completed @@ -1120,7 +1084,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { // NOTE: Both these methods are no-ops for batches that originated from // other clients. this.processUserCallback(batchId, error ? error : null); - this.localStore.removeCachedMutationBatchMetadata(batchId); + removeCachedMutationBatchMetadata(this.localStore, batchId); } else { fail(`Unknown batchState: ${batchState}`); } @@ -1236,7 +1200,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { ); // For queries that never executed on this client, we need to // allocate the target in LocalStore and initialize a new View. - const target = await this.localStore.getTarget(targetId); + const target = await getCachedTarget(this.localStore, targetId); debugAssert(!!target, `Target for id ${targetId} not found`); targetData = await this.localStore.allocateTarget(target); await this.initializeViewAndComputeSnapshot( @@ -1277,7 +1241,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { } getActiveClients(): Promise { - return this.localStore.getActiveClients(); + return getCurrentlyActiveClients(this.localStore); } async applyTargetState( @@ -1296,7 +1260,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { switch (state) { case 'current': case 'not-current': { - const changes = await this.localStore.getNewDocumentChanges(); + const changes = await getNewDocumentChanges(this.localStore); const synthesizedRemoteEvent = RemoteEvent.createSynthesizedRemoteEventForCurrentChange( targetId, state === 'current' @@ -1336,7 +1300,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { continue; } - const target = await this.localStore.getTarget(targetId); + const target = await getCachedTarget(this.localStore, targetId); debugAssert( !!target, `Query data for active target ${targetId} not found` @@ -1370,7 +1334,7 @@ class MultiTabSyncEngineImpl extends SyncEngineImpl { } export function newMultiTabSyncEngine( - localStore: MultiTabLocalStore, + localStore: LocalStore, remoteStore: RemoteStore, datastore: Datastore, sharedClientState: SharedClientState, diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 38e925e18d0..0c95cf148f4 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -42,7 +42,7 @@ import { MutationBatchResult } from '../model/mutation_batch'; import { RemoteEvent, TargetChange } from '../remote/remote_event'; -import { debugAssert, hardAssert } from '../util/assert'; +import { debugAssert, debugCast, hardAssert } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; import { logDebug } from '../util/log'; import { primitiveComparator } from '../util/misc'; @@ -145,9 +145,6 @@ export interface QueryResult { * unrecoverable error (should be caught / reported by the async_queue). */ export interface LocalStore { - /** Starts the LocalStore. */ - start(): Promise; - /** * Tells the LocalStore that the currently authenticated user has changed. * @@ -296,19 +293,19 @@ class LocalStoreImpl implements LocalStore { * The set of all mutations that have been sent but not yet been applied to * the backend. */ - protected mutationQueue: MutationQueue; + mutationQueue: MutationQueue; /** The set of all cached remote documents. */ - protected remoteDocuments: RemoteDocumentCache; + remoteDocuments: RemoteDocumentCache; /** * The "local" view of all documents (layering mutationQueue on top of * remoteDocumentCache). */ - protected localDocuments: LocalDocumentsView; + localDocuments: LocalDocumentsView; /** Maps a target to its `TargetData`. */ - protected targetCache: TargetCache; + targetCache: TargetCache; /** * Maps a targetID to data about its target. @@ -316,9 +313,7 @@ class LocalStoreImpl implements LocalStore { * PORTING NOTE: We are using an immutable data structure on Web to make re-runs * of `applyRemoteEvent()` idempotent. */ - protected targetDataByTarget = new SortedMap( - primitiveComparator - ); + targetDataByTarget = new SortedMap(primitiveComparator); /** Maps a target to its targetID. */ // TODO(wuandy): Evaluate if TargetId can be part of Target. @@ -332,11 +327,11 @@ class LocalStoreImpl implements LocalStore { * * PORTING NOTE: This is only used for multi-tab synchronization. */ - protected lastDocumentChangeReadTime = SnapshotVersion.min(); + lastDocumentChangeReadTime = SnapshotVersion.min(); constructor( /** Manages our in-memory or durable persistence. */ - protected persistence: Persistence, + readonly persistence: Persistence, private queryEngine: QueryEngine, initialUser: User ) { @@ -355,10 +350,6 @@ class LocalStoreImpl implements LocalStore { this.queryEngine.setLocalDocumentsView(this.localDocuments); } - start(): Promise { - return Promise.resolve(); - } - async handleUserChange(user: User): Promise { let newMutationQueue = this.mutationQueue; let newLocalDocuments = this.localDocuments; @@ -1057,152 +1048,137 @@ export function newLocalStore( return new LocalStoreImpl(persistence, queryEngine, initialUser); } -/** - * An interface on top of LocalStore that provides additional functionality - * for MultiTabSyncEngine. - */ -export interface MultiTabLocalStore extends LocalStore { - /** Returns the local view of the documents affected by a mutation batch. */ - lookupMutationDocuments(batchId: BatchId): Promise; - - removeCachedMutationBatchMetadata(batchId: BatchId): void; - - setNetworkEnabled(networkEnabled: boolean): void; - - getActiveClients(): Promise; - - getTarget(targetId: TargetId): Promise; - - /** - * Returns the set of documents that have been updated since the last call. - * If this is the first call, returns the set of changes since client - * initialization. Further invocations will return document changes since - * the point of rejection. - */ - getNewDocumentChanges(): Promise; - - /** - * Reads the newest document change from persistence and forwards the internal - * synchronization marker so that calls to `getNewDocumentChanges()` - * only return changes that happened after client initialization. - */ - synchronizeLastDocumentChangeReadTime(): Promise; +/** Returns the local view of the documents affected by a mutation batch. */ +// PORTING NOTE: Multi-Tab only. +export function lookupMutationDocuments( + localStore: LocalStore, + batchId: BatchId +): Promise { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const mutationQueueImpl = debugCast( + localStoreImpl.mutationQueue, + IndexedDbMutationQueue // We only support IndexedDb in multi-tab mode. + ); + return localStoreImpl.persistence.runTransaction( + 'Lookup mutation documents', + 'readonly', + txn => { + return mutationQueueImpl.lookupMutationKeys(txn, batchId).next(keys => { + if (keys) { + return localStoreImpl.localDocuments.getDocuments( + txn, + keys + ) as PersistencePromise; + } else { + return PersistencePromise.resolve(null); + } + }); + } + ); } -/** - * An implementation of LocalStore that provides additional functionality - * for MultiTabSyncEngine. - * - * Note: some field defined in this class might have public access level, but - * the class is not exported so they are only accessible from this module. - * This is useful to implement optional features (like bundles) in free - * functions, such that they are tree-shakeable. - */ -// PORTING NOTE: Web only. -class MultiTabLocalStoreImpl extends LocalStoreImpl - implements MultiTabLocalStore { - protected mutationQueue: IndexedDbMutationQueue; - protected remoteDocuments: IndexedDbRemoteDocumentCache; - protected targetCache: IndexedDbTargetCache; - - constructor( - protected persistence: IndexedDbPersistence, - queryEngine: QueryEngine, - initialUser: User - ) { - super(persistence, queryEngine, initialUser); - - this.mutationQueue = persistence.getMutationQueue(initialUser); - this.remoteDocuments = persistence.getRemoteDocumentCache(); - this.targetCache = persistence.getTargetCache(); - } +// PORTING NOTE: Multi-Tab only. +export function removeCachedMutationBatchMetadata( + localStore: LocalStore, + batchId: BatchId +): void { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const mutationQueueImpl = debugCast( + localStoreImpl.mutationQueue, + IndexedDbMutationQueue // We only support IndexedDb in multi-tab mode. + ); + mutationQueueImpl.removeCachedMutationKeys(batchId); +} - /** Starts the LocalStore. */ - start(): Promise { - return this.synchronizeLastDocumentChangeReadTime(); - } +// PORTING NOTE: Multi-Tab only. +export function getCurrentlyActiveClients( + localStore: LocalStore +): Promise { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const persistenceImpl = debugCast( + localStoreImpl.persistence, + IndexedDbPersistence // We only support IndexedDb in multi-tab mode. + ); + return persistenceImpl.getActiveClients(); +} - lookupMutationDocuments(batchId: BatchId): Promise { - return this.persistence.runTransaction( - 'Lookup mutation documents', +// PORTING NOTE: Multi-Tab only. +export function getCachedTarget( + localStore: LocalStore, + targetId: TargetId +): Promise { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const targetCacheImpl = debugCast( + localStoreImpl.targetCache, + IndexedDbTargetCache // We only support IndexedDb in multi-tab mode. + ); + const cachedTargetData = localStoreImpl.targetDataByTarget.get(targetId); + if (cachedTargetData) { + return Promise.resolve(cachedTargetData.target); + } else { + return localStoreImpl.persistence.runTransaction( + 'Get target data', 'readonly', txn => { - return this.mutationQueue - .lookupMutationKeys(txn, batchId) - .next(keys => { - if (keys) { - return this.localDocuments.getDocuments( - txn, - keys - ) as PersistencePromise; - } else { - return PersistencePromise.resolve(null); - } - }); + return targetCacheImpl + .getTargetDataForTarget(txn, targetId) + .next(targetData => (targetData ? targetData.target : null)); } ); } +} - removeCachedMutationBatchMetadata(batchId: BatchId): void { - this.mutationQueue.removeCachedMutationKeys(batchId); - } - - setNetworkEnabled(networkEnabled: boolean): void { - this.persistence.setNetworkEnabled(networkEnabled); - } - - getActiveClients(): Promise { - return this.persistence.getActiveClients(); - } - - getTarget(targetId: TargetId): Promise { - const cachedTargetData = this.targetDataByTarget.get(targetId); - - if (cachedTargetData) { - return Promise.resolve(cachedTargetData.target); - } else { - return this.persistence.runTransaction( - 'Get target data', - 'readonly', - txn => { - return this.targetCache - .getTargetDataForTarget(txn, targetId) - .next(targetData => (targetData ? targetData.target : null)); - } - ); - } - } - - getNewDocumentChanges(): Promise { - return this.persistence - .runTransaction('Get new document changes', 'readonly', txn => - this.remoteDocuments.getNewDocumentChanges( - txn, - this.lastDocumentChangeReadTime - ) +/** + * Returns the set of documents that have been updated since the last call. + * If this is the first call, returns the set of changes since client + * initialization. Further invocations will return document changes since + * the point of rejection. + */ +// PORTING NOTE: Multi-Tab only. +export function getNewDocumentChanges( + localStore: LocalStore +): Promise { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const remoteDocumentCacheImpl = debugCast( + localStoreImpl.remoteDocuments, + IndexedDbRemoteDocumentCache // We only support IndexedDb in multi-tab mode. + ); + return localStoreImpl.persistence + .runTransaction('Get new document changes', 'readonly', txn => + remoteDocumentCacheImpl.getNewDocumentChanges( + txn, + localStoreImpl.lastDocumentChangeReadTime ) - .then(({ changedDocs, readTime }) => { - this.lastDocumentChangeReadTime = readTime; - return changedDocs; - }); - } + ) + .then(({ changedDocs, readTime }) => { + localStoreImpl.lastDocumentChangeReadTime = readTime; + return changedDocs; + }); +} - async synchronizeLastDocumentChangeReadTime(): Promise { - this.lastDocumentChangeReadTime = await this.persistence.runTransaction( +/** + * Reads the newest document change from persistence and forwards the internal + * synchronization marker so that calls to `getNewDocumentChanges()` + * only return changes that happened after client initialization. + */ +// PORTING NOTE: Multi-Tab only. +export async function synchronizeLastDocumentChangeReadTime( + localStore: LocalStore +): Promise { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const remoteDocumentCacheImpl = debugCast( + localStoreImpl.remoteDocuments, + IndexedDbRemoteDocumentCache // We only support IndexedDb in multi-tab mode. + ); + return localStoreImpl.persistence + .runTransaction( 'Synchronize last document change read time', 'readonly', - txn => this.remoteDocuments.getLastReadTime(txn) - ); - } -} - -export function newMultiTabLocalStore( - /** Manages our in-memory or durable persistence. */ - persistence: IndexedDbPersistence, - queryEngine: QueryEngine, - initialUser: User -): MultiTabLocalStore { - return new MultiTabLocalStoreImpl(persistence, queryEngine, initialUser); + txn => remoteDocumentCacheImpl.getLastReadTime(txn) + ) + .then(readTime => { + localStoreImpl.lastDocumentChangeReadTime = readTime; + }); } /** diff --git a/packages/firestore/src/local/memory_persistence.ts b/packages/firestore/src/local/memory_persistence.ts index 29173d00ad4..1f73159a8c5 100644 --- a/packages/firestore/src/local/memory_persistence.ts +++ b/packages/firestore/src/local/memory_persistence.ts @@ -108,6 +108,10 @@ export class MemoryPersistence implements Persistence { // No op. } + setNetworkEnabled(): void { + // No op. + } + getIndexManager(): MemoryIndexManager { return this.indexManager; } diff --git a/packages/firestore/src/local/persistence.ts b/packages/firestore/src/local/persistence.ts index 146b0331738..365bfee8c0c 100644 --- a/packages/firestore/src/local/persistence.ts +++ b/packages/firestore/src/local/persistence.ts @@ -185,6 +185,14 @@ export interface Persistence { databaseDeletedListener: () => Promise ): void; + /** + * Adjusts the current network state in the client's metadata, potentially + * affecting the primary lease. + * + * PORTING NOTE: This is only used for Web multi-tab. + */ + setNetworkEnabled(networkEnabled: boolean): void; + /** * Returns a MutationQueue representing the persisted mutations for the * given user. diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index b3bd2adfc44..fd0924900c1 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -120,6 +120,8 @@ class DatastoreImpl extends Datastore { } } +// TODO(firestorexp): Make sure there is only one Datastore instance per +// firestore-exp client. export function newDatastore( credentials: CredentialsProvider, serializer: JsonProtoSerializer diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index d1ba2c8374d..e3503c0cbaa 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -31,7 +31,7 @@ import { LocalStore, LocalWriteResult, newLocalStore, - newMultiTabLocalStore + synchronizeLastDocumentChangeReadTime } from '../../../src/local/local_store'; import { LocalViewChanges } from '../../../src/local/local_view_changes'; import { Persistence } from '../../../src/local/persistence'; @@ -441,12 +441,12 @@ describe('LocalStore w/ IndexedDB Persistence (SimpleQueryEngine)', () => { QueryEngineType.Simple ); const persistence = await persistenceHelpers.testIndexedDbPersistence(); - const localStore = newMultiTabLocalStore( + const localStore = newLocalStore( persistence, queryEngine, User.UNAUTHENTICATED ); - await localStore.start(); + await synchronizeLastDocumentChangeReadTime(localStore); return { queryEngine, persistence, localStore }; } @@ -468,12 +468,12 @@ describe('LocalStore w/ IndexedDB Persistence (IndexFreeQueryEngine)', () => { QueryEngineType.IndexFree ); const persistence = await persistenceHelpers.testIndexedDbPersistence(); - const localStore = newMultiTabLocalStore( + const localStore = newLocalStore( persistence, queryEngine, User.UNAUTHENTICATED ); - await localStore.start(); + await synchronizeLastDocumentChangeReadTime(localStore); return { queryEngine, persistence, localStore }; } diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index eeea6b5e69e..3448ba141b8 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -667,7 +667,8 @@ abstract class TestRunner { // Make sure to execute all writes that are currently queued. This allows us // to assert on the total number of requests sent before shutdown. await this.remoteStore.fillWritePipeline(); - await this.syncEngine.disableNetwork(); + this.persistence.setNetworkEnabled(false); + await this.remoteStore.disableNetwork(); } private async doDrainQueue(): Promise { @@ -676,7 +677,8 @@ abstract class TestRunner { private async doEnableNetwork(): Promise { this.networkEnabled = true; - await this.syncEngine.enableNetwork(); + this.persistence.setNetworkEnabled(true); + await this.remoteStore.enableNetwork(); } private async doShutdown(): Promise { From c28a9f5f3e863e0585df9aa344f52c748181ac36 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 20 Jul 2020 09:43:54 -0700 Subject: [PATCH 2/3] Review --- packages/firestore/src/local/local_store.ts | 35 +++++++++-------- packages/firestore/src/util/assert.ts | 43 +++++++++++++++++---- 2 files changed, 53 insertions(+), 25 deletions(-) diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 0c95cf148f4..878627e2c43 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -1054,9 +1054,10 @@ export function lookupMutationDocuments( localStore: LocalStore, batchId: BatchId ): Promise { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); - const mutationQueueImpl = debugCast( - localStoreImpl.mutationQueue, + const [localStoreImpl, mutationQueueImpl] = debugCast( + localStore, + LocalStoreImpl, + (localStore as LocalStoreImpl).mutationQueue, IndexedDbMutationQueue // We only support IndexedDb in multi-tab mode. ); return localStoreImpl.persistence.runTransaction( @@ -1082,9 +1083,8 @@ export function removeCachedMutationBatchMetadata( localStore: LocalStore, batchId: BatchId ): void { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); const mutationQueueImpl = debugCast( - localStoreImpl.mutationQueue, + debugCast(localStore, LocalStoreImpl).mutationQueue, IndexedDbMutationQueue // We only support IndexedDb in multi-tab mode. ); mutationQueueImpl.removeCachedMutationKeys(batchId); @@ -1094,9 +1094,8 @@ export function removeCachedMutationBatchMetadata( export function getCurrentlyActiveClients( localStore: LocalStore ): Promise { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); const persistenceImpl = debugCast( - localStoreImpl.persistence, + debugCast(localStore, LocalStoreImpl).persistence, IndexedDbPersistence // We only support IndexedDb in multi-tab mode. ); return persistenceImpl.getActiveClients(); @@ -1107,9 +1106,10 @@ export function getCachedTarget( localStore: LocalStore, targetId: TargetId ): Promise { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); - const targetCacheImpl = debugCast( - localStoreImpl.targetCache, + const [localStoreImpl, targetCacheImpl] = debugCast( + localStore, + LocalStoreImpl, + (localStore as LocalStoreImpl).targetCache, IndexedDbTargetCache // We only support IndexedDb in multi-tab mode. ); const cachedTargetData = localStoreImpl.targetDataByTarget.get(targetId); @@ -1131,16 +1131,17 @@ export function getCachedTarget( /** * Returns the set of documents that have been updated since the last call. * If this is the first call, returns the set of changes since client - * initialization. Further invocations will return document changes since - * the point of rejection. + * initialization. Further invocations will return document that have changed + * since the prior call. */ // PORTING NOTE: Multi-Tab only. export function getNewDocumentChanges( localStore: LocalStore ): Promise { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); - const remoteDocumentCacheImpl = debugCast( - localStoreImpl.remoteDocuments, + const [localStoreImpl, remoteDocumentCacheImpl] = debugCast( + localStore, + LocalStoreImpl, + (localStore as LocalStoreImpl).remoteDocuments, IndexedDbRemoteDocumentCache // We only support IndexedDb in multi-tab mode. ); return localStoreImpl.persistence @@ -1157,8 +1158,8 @@ export function getNewDocumentChanges( } /** - * Reads the newest document change from persistence and forwards the internal - * synchronization marker so that calls to `getNewDocumentChanges()` + * Reads the newest document change from persistence and moves the internal + * synchronization marker forward so that calls to `getNewDocumentChanges()` * only return changes that happened after client initialization. */ // PORTING NOTE: Multi-Tab only. diff --git a/packages/firestore/src/util/assert.ts b/packages/firestore/src/util/assert.ts index eb4b728be46..bc7e2c37c9b 100644 --- a/packages/firestore/src/util/assert.ts +++ b/packages/firestore/src/util/assert.ts @@ -72,17 +72,44 @@ export function debugAssert( } /** - * Casts `obj` to `T`. In non-production builds, verifies that `obj` is an - * instance of `T` before casting. + * Casts `obj1` to `S` and `obj2` to `T`. In non-production builds, + * verifies that `obj1` and `obj2` are instances of `S` and `T` before casting. */ -export function debugCast( +export function debugCast( + obj1: object, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + constructor1: { new (...args: any[]): S }, + obj2: object, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + constructor2: { new (...args: any[]): T } +): [S, T] | never; +/** + * Casts `obj` to `S`. In non-production builds, verifies that `obj` is an + * instance of `S` before casting. + */ +export function debugCast( obj: object, // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor: { new (...args: any[]): T } -): T | never { + constructor: { new (...args: any[]): S } +): S | never; +export function debugCast( + obj1: object, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + constructor1: { new (...args: any[]): S }, + obj2?: object, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + constructor2?: { new (...args: any[]): T } +): S | [S, T] | never { + debugAssert( + obj1 instanceof constructor1, + `Expected type '${constructor1.name}', but was '${obj1.constructor.name}'` + ); + if (!obj2 || !constructor2) { + return obj1 as S; + } debugAssert( - obj instanceof constructor, - `Expected type '${constructor.name}', but was '${obj.constructor.name}'` + obj2 instanceof constructor2, + `Expected type '${constructor2.name}', but was '${obj2.constructor.name}'` ); - return obj as T; + return [obj1 as S, obj2 as T]; } From 08f927aef49f01ff7134ee61fc2f84a7ac612466 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 20 Jul 2020 09:46:43 -0700 Subject: [PATCH 3/3] Review review --- packages/firestore/src/local/local_store.ts | 21 +++++----- packages/firestore/src/util/assert.ts | 43 ++++----------------- 2 files changed, 17 insertions(+), 47 deletions(-) diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 878627e2c43..c25c8542ef4 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -1054,10 +1054,9 @@ export function lookupMutationDocuments( localStore: LocalStore, batchId: BatchId ): Promise { - const [localStoreImpl, mutationQueueImpl] = debugCast( - localStore, - LocalStoreImpl, - (localStore as LocalStoreImpl).mutationQueue, + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const mutationQueueImpl = debugCast( + localStoreImpl.mutationQueue, IndexedDbMutationQueue // We only support IndexedDb in multi-tab mode. ); return localStoreImpl.persistence.runTransaction( @@ -1106,10 +1105,9 @@ export function getCachedTarget( localStore: LocalStore, targetId: TargetId ): Promise { - const [localStoreImpl, targetCacheImpl] = debugCast( - localStore, - LocalStoreImpl, - (localStore as LocalStoreImpl).targetCache, + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const targetCacheImpl = debugCast( + localStoreImpl.targetCache, IndexedDbTargetCache // We only support IndexedDb in multi-tab mode. ); const cachedTargetData = localStoreImpl.targetDataByTarget.get(targetId); @@ -1138,10 +1136,9 @@ export function getCachedTarget( export function getNewDocumentChanges( localStore: LocalStore ): Promise { - const [localStoreImpl, remoteDocumentCacheImpl] = debugCast( - localStore, - LocalStoreImpl, - (localStore as LocalStoreImpl).remoteDocuments, + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + const remoteDocumentCacheImpl = debugCast( + localStoreImpl.remoteDocuments, IndexedDbRemoteDocumentCache // We only support IndexedDb in multi-tab mode. ); return localStoreImpl.persistence diff --git a/packages/firestore/src/util/assert.ts b/packages/firestore/src/util/assert.ts index bc7e2c37c9b..eb4b728be46 100644 --- a/packages/firestore/src/util/assert.ts +++ b/packages/firestore/src/util/assert.ts @@ -72,44 +72,17 @@ export function debugAssert( } /** - * Casts `obj1` to `S` and `obj2` to `T`. In non-production builds, - * verifies that `obj1` and `obj2` are instances of `S` and `T` before casting. + * Casts `obj` to `T`. In non-production builds, verifies that `obj` is an + * instance of `T` before casting. */ -export function debugCast( - obj1: object, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor1: { new (...args: any[]): S }, - obj2: object, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor2: { new (...args: any[]): T } -): [S, T] | never; -/** - * Casts `obj` to `S`. In non-production builds, verifies that `obj` is an - * instance of `S` before casting. - */ -export function debugCast( +export function debugCast( obj: object, // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor: { new (...args: any[]): S } -): S | never; -export function debugCast( - obj1: object, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor1: { new (...args: any[]): S }, - obj2?: object, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - constructor2?: { new (...args: any[]): T } -): S | [S, T] | never { - debugAssert( - obj1 instanceof constructor1, - `Expected type '${constructor1.name}', but was '${obj1.constructor.name}'` - ); - if (!obj2 || !constructor2) { - return obj1 as S; - } + constructor: { new (...args: any[]): T } +): T | never { debugAssert( - obj2 instanceof constructor2, - `Expected type '${constructor2.name}', but was '${obj2.constructor.name}'` + obj instanceof constructor, + `Expected type '${constructor.name}', but was '${obj.constructor.name}'` ); - return [obj1 as S, obj2 as T]; + return obj as T; }