addressing comments #1

Author: wu-hui

packages/firebase/index.d.ts

@@ -6225,11 +6225,7 @@ declare namespace firebase.firestore {
     /**
      * @hidden
      */
-    INTERNAL: {
-      delete: () => Promise<void>;
-      shutdown: () => Promise<void>;
-      isShutdown: () => boolean;
-    };
+    INTERNAL: { delete: () => Promise<void> };
   }
 
   /**

packages/firestore-types/index.d.ts

@@ -276,11 +276,7 @@ export class FirebaseFirestore {
    */
   disableNetwork(): Promise<void>;
 
-  INTERNAL: {
-    delete: () => Promise<void>;
-    shutdown: () => Promise<void>;
-    isShutdown: () => boolean;
-  };
+  INTERNAL: { delete: () => Promise<void> };
 }
 
 /**

packages/firestore/src/api/database.ts

@@ -447,6 +447,39 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     return deferred.promise;
   }
 
+  /**
+     * Shuts down this Firestore instance.
+     *
+     * After shutdown only the `clearPersistence()` method may be used. Any other method
+     * will throw a `FirestoreError`.
+     *
+     * To restart after shutdown, simply create a new instance of FirebaseFirestore with
+     * `firebase.firestore()`.
+     *
+     * Shutdown does not cancel any pending writes and any promises that are awaiting a response
+     * from the server will not be resolved. If you have persistence enabled, the next time you
+     * start this instance, it will resume attempting to send these writes to the server.
+     *
+     * Note: Under normal circumstances, calling `shutdown()` is not required. This
+     * method is useful only when you want to force this instance to release all of its resources or
+     * in combination with `clearPersistence()` to ensure that all local state is destroyed
+     * between test runs.
+     *
+     * @return A promise that is resolved when the instance has been successfully shut down.
+     */
+    // TODO(b/135755126): make this public.
+    _shutdown(): Promise<void> {
+      (this.app as _FirebaseApp)._removeServiceInstance(
+        'firestore'
+      );
+      return this.INTERNAL.delete();
+    }
+
+    get _isShutdown(): boolean {
+      this.ensureClientConfigured();
+      return this._firestoreClient!.clientShutdown;
+    }
+
   ensureClientConfigured(): FirestoreClient {
     if (!this._firestoreClient) {
       // Kick off starting the client but don't actually wait for it.
@@ -538,44 +571,11 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
   }
 
   INTERNAL = {
-    // TODO(b/135755126): Make this public
     delete: async (): Promise<void> => {
       // The client must be initalized to ensure that all subsequent API usage
       // throws an exception.
       this.ensureClientConfigured();
       await this._firestoreClient!.shutdown();
-    },
-
-    /**
-     * Shuts down this Firestore instance.
-     *
-     * After shutdown only the `clearPersistence()` method may be used. Any other method
-     * will throw an `FirestoreError`.
-     *
-     * To restart after shutdown, simply create a new instance of FirebaseFirestore with
-     * `Firebase.firestore()`.
-     *
-     * Shutdown does not cancel any pending writes and any promises that are awaiting a response
-     * from the server will not be resolved. The next time you start this instance,
-     * it will resume attempting to send these writes to the server.
-     *
-     * Note: Under normal circumstances, calling `shutdown()` is not required. This
-     * method is useful only when you want to force this instance to release all of its resources or
-     * in combination with `clearPersistence()` to ensure that all local state is destroyed
-     * between test runs.
-     *
-     * @return A promise that is resolved when the instance has been successfully shut down.
-     */
-    // TODO(b/135755126): make this public.
-    shutdown: (): Promise<void> => {
-      (this._config.firebaseApp as _FirebaseApp)._removeServiceInstance(
-        'firestore'
-      );
-      return this.INTERNAL.delete();
-    },
-
-    isShutdown: (): boolean => {
-      return this._firestoreClient!.clientShutdown;
     }
   };
 

packages/firestore/src/core/firestore_client.ts

@@ -506,7 +506,7 @@ export class FirestoreClient {
   }
 
   shutdown(): Promise<void> {
-    return this.asyncQueue.enqueueAndInitilizeShutdown(async () => {
+    return this.asyncQueue.enqueueAndInitiateShutdown(async () => {
       // PORTING NOTE: LocalStore does not need an explicit shutdown on web.
       if (this.lruScheduler) {
         this.lruScheduler.stop();

packages/firestore/src/util/async_queue.ts

@@ -226,7 +226,6 @@ export class AsyncQueue {
     op: () => Promise<T>
   ): void {
     this.verifyNotFailed();
-    // tslint:disable-next-line:no-floating-promises
     this.enqueueInternal(op);
   }
 
@@ -238,7 +237,6 @@ export class AsyncQueue {
     op: () => Promise<T>
   ): Promise<T> {
     this.verifyNotFailed();
-    // tslint:disable-next-line:no-floating-promises
     return this.enqueueInternal(op);
   }
 
@@ -249,18 +247,12 @@ export class AsyncQueue {
    * Once this method is called, the only possible way to request running an operation
    * is through `enqueueAndForgetEvenAfterShutdown`.
    */
-  enqueueAndInitilizeShutdown<T extends unknown>(
-    op: () => Promise<T>
-  ): Promise<T> {
+  async enqueueAndInitiateShutdown(op: () => Promise<void>): Promise<void> {
     this.verifyNotFailed();
-    if (this._isShuttingDown) {
-      // Return a Promise resolves right away if it is already shutdown.
-      return new Promise<T>(resolve => resolve(undefined));
+    if (!this._isShuttingDown) {
+      this._isShuttingDown = true;
+      await this.enqueueEvenAfterShutdown(op);
     }
-
-    const promise = this.enqueueInternal(op);
-    this._isShuttingDown = true;
-    return promise;
   }
 
   /**

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

@@ -1073,7 +1073,7 @@ apiDescribe('Database', (persistence: boolean) => {
   it('can start a new instance after shut down', async () => {
     return withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
-      await firestore.INTERNAL.shutdown();
+      await (firestore as any)._shutdown();
 
       const newFirestore = firebase.firestore!(firestore.app);
       expect(newFirestore).to.not.equal(firestore);
@@ -1092,14 +1092,14 @@ apiDescribe('Database', (persistence: boolean) => {
       const app = docRef.firestore.app;
       await app.delete();
 
-      expect(docRef.firestore.INTERNAL.isShutdown()).to.be.true;
+      expect((docRef.firestore as any)._isShutdown).to.be.true;
     });
   });
 
   it('new operation after shutdown should throw', async () => {
     await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
-      await firestore.INTERNAL.shutdown();
+      await (firestore as any)._shutdown();
 
       expect(() => {
         firestore.doc(docRef.path).set({ foo: 'bar' });
@@ -1110,8 +1110,8 @@ apiDescribe('Database', (persistence: boolean) => {
   it('calling shutdown mutiple times should proceed', async () => {
     await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
-      await firestore.INTERNAL.shutdown();
-      await firestore.INTERNAL.shutdown();
+      await (firestore as any)._shutdown();
+      await (firestore as any)._shutdown();
 
       expect(() => {
         firestore.doc(docRef.path).set({ foo: 'bar' });

packages/firestore/test/unit/util/async_queue.test.ts

@@ -213,18 +213,17 @@ describe('AsyncQueue', () => {
   it('Schedules operaions with respect to shut down', async () => {
     const queue = new AsyncQueue();
     const completedSteps: number[] = [];
-    const doStep = (n: number): Promise<number> =>
+    const doStep = (n: number): Promise<void> =>
       defer(() => {
-        const result = completedSteps.push(n);
-        return result;
+        completedSteps.push(n);
       });
 
     queue.enqueueAndForget(() => doStep(1));
 
     // After this call, only operations requested via
     // `enqueueAndForgetEvenAfterShutdown` gets executed.
     // tslint:disable-next-line:no-floating-promises
-    queue.enqueueAndInitilizeShutdown(() => doStep(2));
+    queue.enqueueAndInitiateShutdown(() => doStep(2));
     queue.enqueueAndForget(() => doStep(3));
     queue.enqueueAndForgetEvenAfterShutdown(() => doStep(4));