addressing comments

Author: wu-hui

packages/firestore/src/api/database.ts

@@ -482,7 +482,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
    * Waits until all currently pending writes for the active user have been acknowledged by the
    * backend.
    *
-   * The returned Promise completes immediately if there are no outstanding writes. Otherwise, the
+   * The returned Promise resolves immediately if there are no outstanding writes. Otherwise, the
    * Task waits for all previously issued writes (including those written in a previous app
    * session), but it does not wait for writes that were added after the method is called. If you
    * wish to wait for additional writes, you have to call `waitForPendingWrites()` again.
@@ -493,6 +493,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
    * acknowledged by the backend.
    */
   _waitForPendingWrites(): Promise<void> {
+    this.ensureClientConfigured();
     return this._firestoreClient!.waitForPendingWrites();
   }
 

packages/firestore/src/core/firestore_client.ts

@@ -523,8 +523,9 @@ export class FirestoreClient {
   }
 
   /**
-   * Returns a `Promise` resolves when all the pending writes at the time when this method is called
-   * received server acknowledgement. An acknowledgement can be either acceptance or rejections.
+   * Returns a Promise that resolves when when all writes that were pending at the time when this
+   * method is called received server acknowledgement. An acknowledgement can be either acceptance
+   * or rejections.
    */
   waitForPendingWrites(): Promise<void> {
     this.verifyNotShutdown();
@@ -550,6 +551,7 @@ export class FirestoreClient {
   }
 
   unlisten(listener: QueryListener): void {
+    this.verifyNotShutdown();
     this.asyncQueue.enqueueAndForget(() => {
       return this.eventMgr.unlisten(listener);
     });

packages/firestore/src/core/sync_engine.ts

@@ -610,9 +610,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   }
 
   /**
-   * Takes a snapshot of current mutation queue, and register a user callback
-   * which will be called when all those mutations in the snapshot are either
-   * accepted or rejected by the server.
+   * Registers a user callback with all the pending mutations at the moment of calling.
+   * When all those mutations are either accepted or rejected by the server, the
+   * registered callback will be triggered.
    */
   async registerPendingWritesCallback(callback: Deferred<void>): Promise<void> {
     if (!this.remoteStore.canUseNetwork()) {
@@ -623,22 +623,20 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       );
     }
 
-    const largestBatchId = await this.localStore.getHighestUnacknowledgedBatchId();
-
-    if (largestBatchId === BATCHID_UNKNOWN) {
+    const highestBatchId = await this.localStore.getHighestUnacknowledgedBatchId();
+    if (highestBatchId === BATCHID_UNKNOWN) {
       // Trigger the callback right away if there is no pending writes at the moment.
       callback.resolve();
-      return Promise.resolve();
+      return;
     }
 
-    const callbacks = this.pendingWritesCallbacks.get(largestBatchId) || [];
+    const callbacks = this.pendingWritesCallbacks.get(highestBatchId) || [];
     callbacks.push(callback);
-    this.pendingWritesCallbacks.set(largestBatchId, callbacks);
-    return Promise.resolve();
+    this.pendingWritesCallbacks.set(highestBatchId, callbacks);
   }
 
   /**
-   * Triggers callbacks waiting for this batch id to get acknowledged by server,
+   * Triggers the callbacks that are waiting for this batch id to get acknowledged by server,
    * if there are any.
    */
   private triggerPendingWritesCallbacks(batchId: BatchId): void {
@@ -650,13 +648,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   }
 
   /** Reject all outstanding callbacks waiting for pending writes to complete. */
-  private rejectOutstandingPendingWritesCallbacks(): void {
+  private rejectOutstandingPendingWritesCallbacks(errorMessage: string): void {
     this.pendingWritesCallbacks.forEach(callbacks => {
       callbacks.forEach(callback => {
         callback.reject(
           new FirestoreError(
-            Code.CANCELLED,
-            "'waitForPendingWrites' task is cancelled due to User change."
+            Code.CANCELLED, errorMessage
           )
         );
       });
@@ -863,7 +860,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
     if (userChanged) {
       // Fails tasks waiting for pending writes requested by previous user.
-      this.rejectOutstandingPendingWritesCallbacks();
+      this.rejectOutstandingPendingWritesCallbacks(
+            "'waitForPendingWrites' promise is rejected due to a user change."
+      );
 
       const result = await this.localStore.handleUserChange(user);
       // TODO(b/114226417): Consider calling this only in the primary tab.

packages/firestore/src/local/indexeddb_mutation_queue.ts

@@ -268,8 +268,7 @@ export class IndexedDbMutationQueue implements MutationQueue {
   getHighestUnacknowledgedBatchId(
     transaction: PersistenceTransaction
   ): PersistencePromise<BatchId> {
-    const range = IDBKeyRange.bound(
-      [this.userId, Number.NEGATIVE_INFINITY],
+    const range = IDBKeyRange.upperBound(
       [this.userId, Number.POSITIVE_INFINITY]
     );
 

packages/firestore/test/integration/api/database.test.ts

@@ -1101,21 +1101,6 @@ apiDescribe('Database', (persistence: boolean) => {
     });
   });
 
-  it('can unlisten queries after shutdown', async () => {
-    return withTestDoc(persistence, async docRef => {
-      const firestore = docRef.firestore;
-      const accumulator = new EventsAccumulator<firestore.DocumentSnapshot>();
-      const unsubscribe = docRef.onSnapshot(accumulator.storeEvent);
-      await accumulator.awaitEvent();
-      await shutdownDb(firestore);
-
-      // This should proceed without error.
-      unsubscribe();
-      // Multiple calls should proceed as well.
-      unsubscribe();
-    });
-  });
-
   it('new operation after shutdown should throw', async () => {
     await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
@@ -1139,24 +1124,19 @@ apiDescribe('Database', (persistence: boolean) => {
     });
   });
 
-  it('can wait for pending writes as expected', async () => {
+  it('can wait for pending writes', async () => {
     await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
       // Prevent pending writes receiving acknowledgement.
       await firestore.disableNetwork();
 
-      const awaitPendingWrites1 = waitForPendingWrites(firestore);
       const pendingWrites = docRef.set({ foo: 'bar' });
-      const awaitPendingWrites2 = waitForPendingWrites(firestore);
-
-      // `awaitsPendingWrites1` resolves immediately because there are no pending writes at
-      // the time it is created.
-      await expect(awaitPendingWrites1).to.be.eventually.fulfilled;
+      const awaitPendingWrites = waitForPendingWrites(firestore);
 
       // pending writes can receive acknowledgements now.
       await firestore.enableNetwork();
-      await expect(pendingWrites).to.be.eventually.fulfilled;
-      await expect(awaitPendingWrites2).to.be.eventually.fulfilled;
+      await pendingWrites;
+      await awaitPendingWrites;
     });
   });
 
@@ -1167,9 +1147,11 @@ apiDescribe('Database', (persistence: boolean) => {
       db.doc('abc/123').set({ foo: 'bar' });
       const awaitPendingWrite = waitForPendingWrites(db);
 
-      mockCredentialsProvider.changeUserTo(new User('user_1'));
+      mockCredentialsProvider.triggerUserChange(new User('user_1'));
 
-      await expect(awaitPendingWrite).to.be.eventually.rejected;
+      await expect(awaitPendingWrite).to.be.eventually.rejectedWith(
+        "'waitForPendingWrites' promise is rejected due to a user change."
+      );
     });
   });
 
@@ -1180,8 +1162,10 @@ apiDescribe('Database', (persistence: boolean) => {
         // Prevent pending writes receiving acknowledgement.
         await firestore.disableNetwork();
 
+      // `awaitsPendingWrites` is created when there is no pending writes, it will resolve
+      // immediately even if we are offline.
         const awaitPendingWrites = waitForPendingWrites(firestore);
-        await expect(awaitPendingWrites).to.be.eventually.fulfilled;
+        await awaitPendingWrites;
       });
     });
 });

packages/firestore/test/integration/util/helpers.ts

@@ -130,8 +130,8 @@ export class MockCredentialsProvider extends EmptyCredentialsProvider {
 
   private listener: CredentialChangeListener | null = null;
 
-  changeUserTo(user: User): void {
-    this.listener!(user);
+  triggerUserChange(newUser: User): void {
+    this.listener!(newUser);
   }
 
   setChangeListener(listener: CredentialChangeListener): void {
@@ -185,8 +185,8 @@ export function withMockCredentialProviderTestDb(
 ): Promise<void> {
   const mockCredentialsProvider = new MockCredentialsProvider();
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  const settings: any = { ...DEFAULT_SETTINGS };
-  settings.credentials = { client: mockCredentialsProvider, type: 'provider' };
+  const settings = {... DEFAULT_SETTINGS,
+    credentials: { client: mockCredentialsProvider, type: 'provider' }};
   return withTestDbsSettings(persistence, DEFAULT_PROJECT_ID, settings, 1,
     ([db]) => {
       return fn(db, mockCredentialsProvider);