Address comments

Author: wu-hui

packages/firestore/src/local/local_store.ts

@@ -153,6 +153,7 @@ export interface LocalStore {
   // change.
   handleUserChange(user: User): Promise<UserChangeResult>;
 
+  /* Accept locally generated Mutations and commit them to storage. */
   localWrite(mutations: Mutation[]): Promise<LocalWriteResult>;
 
   /**
@@ -180,7 +181,9 @@ export interface LocalStore {
   rejectBatch(batchId: BatchId): Promise<MaybeDocumentMap>;
 
   /**
-   * Returns the largest (latest) batch id in mutation queue that is pending server response.
+   * Returns the largest (latest) batch id in mutation queue that is pending
+   * server response.
+   *
    * Returns `BATCHID_UNKNOWN` if the queue is empty.
    */
   getHighestUnacknowledgedBatchId(): Promise<BatchId>;
@@ -270,9 +273,9 @@ export interface LocalStore {
  * Implements `LocalStore` interface.
  *
  * 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 file. This is
- * useful to implement optional features (like bundles) in free functions, such
- * that they are tree-shakeable.
+ * 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.
  */
 class LocalStoreImpl implements LocalStore {
   /**
@@ -347,19 +350,10 @@ class LocalStoreImpl implements LocalStore {
     this.queryEngine.setLocalDocumentsView(this.localDocuments);
   }
 
-  /** Starts the LocalStore. */
   start(): Promise<void> {
     return Promise.resolve();
   }
 
-  /**
-   * Tells the LocalStore that the currently authenticated user has changed.
-   *
-   * In response the local store switches the mutation queue to the new user and
-   * returns any resulting document changes.
-   */
-  // PORTING NOTE: Android and iOS only return the documents affected by the
-  // change.
   async handleUserChange(user: User): Promise<UserChangeResult> {
     let newMutationQueue = this.mutationQueue;
     let newLocalDocuments = this.localDocuments;
@@ -430,7 +424,6 @@ class LocalStoreImpl implements LocalStore {
     return result;
   }
 
-  /* Accept locally generated Mutations and commit them to storage. */
   localWrite(mutations: Mutation[]): Promise<LocalWriteResult> {
     const localWriteTime = Timestamp.now();
     const keys = mutations.reduce(
@@ -488,20 +481,6 @@ class LocalStoreImpl implements LocalStore {
       });
   }
 
-  /**
-   * Acknowledge the given batch.
-   *
-   * On the happy path when a batch is acknowledged, the local store will
-   *
-   *  + remove the batch from the mutation queue;
-   *  + apply the changes to the remote document cache;
-   *  + recalculate the latency compensated view implied by those changes (there
-   *    may be mutations in the queue that affect the documents but haven't been
-   *    acknowledged yet); and
-   *  + give the changed documents back the sync engine
-   *
-   * @returns The resulting (modified) documents.
-   */
   acknowledgeBatch(
     batchResult: MutationBatchResult
   ): Promise<MaybeDocumentMap> {
@@ -525,12 +504,6 @@ class LocalStoreImpl implements LocalStore {
     );
   }
 
-  /**
-   * Remove mutations from the MutationQueue for the specified batch;
-   * LocalDocuments will be recalculated.
-   *
-   * @returns The resulting modified documents.
-   */
   rejectBatch(batchId: BatchId): Promise<MaybeDocumentMap> {
     return this.persistence.runTransaction(
       'Reject batch',
@@ -554,10 +527,6 @@ class LocalStoreImpl implements LocalStore {
     );
   }
 
-  /**
-   * Returns the largest (latest) batch id in mutation queue that is pending server response.
-   * Returns `BATCHID_UNKNOWN` if the queue is empty.
-   */
   getHighestUnacknowledgedBatchId(): Promise<BatchId> {
     return this.persistence.runTransaction(
       'Get highest unacknowledged batch id',
@@ -568,10 +537,6 @@ class LocalStoreImpl implements LocalStore {
     );
   }
 
-  /**
-   * Returns the last consistent snapshot processed (used by the RemoteStore to
-   * determine whether to buffer incoming snapshots from the backend).
-   */
   getLastRemoteSnapshotVersion(): Promise<SnapshotVersion> {
     return this.persistence.runTransaction(
       'Get last remote snapshot version',
@@ -580,14 +545,6 @@ class LocalStoreImpl implements LocalStore {
     );
   }
 
-  /**
-   * Update the "ground-state" (remote) documents. We assume that the remote
-   * event reflects any write batches that have been acknowledged or rejected
-   * (i.e. we do not re-apply local mutations to updates from this event).
-   *
-   * LocalDocuments are re-calculated if there are remaining mutations in the
-   * queue.
-   */
   applyRemoteEvent(remoteEvent: RemoteEvent): Promise<MaybeDocumentMap> {
     const remoteVersion = remoteEvent.snapshotVersion;
     let newTargetDataByTargetMap = this.targetDataByTarget;
@@ -801,9 +758,6 @@ class LocalStoreImpl implements LocalStore {
     return changes > 0;
   }
 
-  /**
-   * Notify local store of the changed views to locally pin documents.
-   */
   async notifyLocalViewChanges(viewChanges: LocalViewChanges[]): Promise<void> {
     try {
       await this.persistence.runTransaction(
@@ -871,12 +825,6 @@ class LocalStoreImpl implements LocalStore {
     }
   }
 
-  /**
-   * Gets the mutation batch after the passed in batchId in the mutation queue
-   * or null if empty.
-   * @param afterBatchId If provided, the batch to search after.
-   * @returns The next mutation or null if there wasn't one.
-   */
   nextMutationBatch(afterBatchId?: BatchId): Promise<MutationBatch | null> {
     return this.persistence.runTransaction(
       'Get next mutation batch',
@@ -893,24 +841,12 @@ class LocalStoreImpl implements LocalStore {
     );
   }
 
-  /**
-   * Read the current value of a Document with a given key or null if not
-   * found - used for testing.
-   */
   readDocument(key: DocumentKey): Promise<MaybeDocument | null> {
     return this.persistence.runTransaction('read document', 'readonly', txn => {
       return this.localDocuments.getDocument(txn, key);
     });
   }
 
-  /**
-   * Assigns the given target an internal ID so that its results can be pinned so
-   * they don't get GC'd. A target must be allocated in the local store before
-   * the store can be used to manage its view.
-   *
-   * Allocating an already allocated `Target` will return the existing `TargetData`
-   * for that `Target`.
-   */
   allocateTarget(target: Target): Promise<TargetData> {
     return this.persistence
       .runTransaction('Allocate target', 'readwrite', txn => {
@@ -961,11 +897,6 @@ class LocalStoreImpl implements LocalStore {
       });
   }
 
-  /**
-   * Returns the TargetData as seen by the LocalStore, including updates that may
-   * have not yet been persisted to the TargetCache.
-   */
-  // Visible for testing.
   getTargetData(
     transaction: PersistenceTransaction,
     target: Target
@@ -980,14 +911,6 @@ class LocalStoreImpl implements LocalStore {
     }
   }
 
-  /**
-   * Unpin all the documents associated with the given target. If
-   * `keepPersistedTargetData` is set to false and Eager GC enabled, the method
-   * directly removes the associated target data from the target cache.
-   *
-   * Releasing a non-existing `Target` is a no-op.
-   */
-  // PORTING NOTE: `keepPersistedTargetData` is multi-tab only.
   releaseTarget(
     targetId: number,
     keepPersistedTargetData: boolean
@@ -1016,14 +939,6 @@ class LocalStoreImpl implements LocalStore {
       });
   }
 
-  /**
-   * Runs the specified query against the local store and returns the results,
-   * potentially taking advantage of query data from previous executions (such
-   * as the set of remote keys).
-   *
-   * @param usePreviousResults Whether results from previous executions can
-   * be used to optimize this query execution.
-   */
   executeQuery(
     query: Query,
     usePreviousResults: boolean
@@ -1160,9 +1075,9 @@ export interface MultiTabLocalStore extends LocalStore {
  * 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 file. This is
- * useful to implement optional features (like bundles) in free functions, such
- * that they are tree-shakeable.
+ * 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
@@ -1188,7 +1103,6 @@ class MultiTabLocalStoreImpl extends LocalStoreImpl
     return this.synchronizeLastDocumentChangeReadTime();
   }
 
-  /** Returns the local view of the documents affected by a mutation batch. */
   lookupMutationDocuments(batchId: BatchId): Promise<MaybeDocumentMap | null> {
     return this.persistence.runTransaction(
       'Lookup mutation documents',
@@ -1240,12 +1154,6 @@ class MultiTabLocalStoreImpl extends LocalStoreImpl
     }
   }
 
-  /**
-   * 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<MaybeDocumentMap> {
     return this.persistence
       .runTransaction('Get new document changes', 'readonly', txn =>
@@ -1260,11 +1168,6 @@ class MultiTabLocalStoreImpl extends LocalStoreImpl
       });
   }
 
-  /**
-   * 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.
-   */
   async synchronizeLastDocumentChangeReadTime(): Promise<void> {
     this.lastDocumentChangeReadTime = await this.persistence.runTransaction(
       'Synchronize last document change read time',

packages/firestore/test/unit/local/local_store.test.ts

@@ -30,7 +30,8 @@ import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
 import {
   LocalStore,
   LocalWriteResult,
-  newLocalStore, newMultiTabLocalStore
+  newLocalStore,
+  newMultiTabLocalStore
 } from '../../../src/local/local_store';
 import { LocalViewChanges } from '../../../src/local/local_view_changes';
 import { Persistence } from '../../../src/local/persistence';