delete services that implement the new FirebaseService interface (#3601)

* delete services that implement the next FirebaseService interface in @firebase/component correctly

* Create small-grapes-teach.md

* debug CI issue

* fix the delete loop

* add eslint suppress

* test

* comment out imports

* Revert "test"

This reverts commit f1b0b44.

* Revert "comment out imports"

This reverts commit 57caf91.

* Update small-grapes-teach.md

* rename delete to _delete

* revert prettier

* change delete to _delete

* fix test

Author: Feiyang1

.changeset/small-grapes-teach.md

@@ -0,0 +1,5 @@
+---
+"@firebase/component": patch
+---
+
+Correctly delete services created by modular SDKs when calling provider.delete()

packages-exp/app-exp/src/api.test.ts

@@ -29,7 +29,10 @@ import {
   onLog
 } from './api';
 import { DEFAULT_ENTRY_NAME } from './constants';
-import { _FirebaseAppInternal } from '@firebase/app-types-exp';
+import {
+  _FirebaseAppInternal,
+  _FirebaseService
+} from '@firebase/app-types-exp';
 import {
   _clearComponents,
   _components,
@@ -175,6 +178,33 @@ describe('API tests', () => {
       deleteApp(app).catch(() => {});
       expect(getApps().length).to.equal(0);
     });
+
+    it('waits for all services being deleted', async () => {
+      _clearComponents();
+      let count = 0;
+      const comp1 = new Component(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        'test1' as any,
+        _container =>
+          ({
+            _delete: async () => {
+              await Promise.resolve();
+              expect(count).to.equal(0);
+              count++;
+            }
+          } as _FirebaseService),
+        ComponentType.PUBLIC
+      );
+      _registerComponent(comp1);
+
+      const app = initializeApp({});
+      // create service instance
+      const test1Provider = _getProvider(app, 'test1' as any);
+      test1Provider.getImmediate();
+
+      await deleteApp(app);
+      expect(count).to.equal(1);
+    });
   });
 
   describe('registerVersion', () => {

packages-exp/app-exp/test/util.ts

@@ -25,7 +25,7 @@ export class TestService implements _FirebaseService {
     return this.app_;
   }
 
-  delete(): Promise<void> {
+  _delete(): Promise<void> {
     return new Promise((resolve: (v?: void) => void) => {
       setTimeout(() => resolve(), 10);
     });

packages-exp/app-types-exp/index.d.ts

@@ -113,7 +113,7 @@ export interface _FirebaseService {
    * Delete the service and free it's resources - called from
    * {@link @firebase/app-exp#deleteApp | deleteApp()}
    */
-  delete(): Promise<void>;
+  _delete(): Promise<void>;
 }
 
 export interface VersionService {

packages-exp/functions-exp/src/service.ts

@@ -95,7 +95,7 @@ export class FunctionsService implements _FirebaseService {
     });
   }
 
-  delete(): Promise<void> {
+  _delete(): Promise<void> {
     return this.deleteService();
   }
 

packages/component/src/provider.test.ts

@@ -19,6 +19,7 @@ import { expect } from 'chai';
 import { fake, SinonSpy } from 'sinon';
 import { ComponentContainer } from './component_container';
 import { FirebaseService } from '@firebase/app-types/private';
+import { _FirebaseService } from '@firebase/app-types-exp';
 import { Provider } from './provider';
 import { getFakeApp, getFakeComponent } from '../test/util';
 import '../test/setup';
@@ -202,7 +203,7 @@ describe('Provider', () => {
     });
 
     describe('delete()', () => {
-      it('calls delete() on the service instance that implements FirebaseService', () => {
+      it('calls delete() on the service instance that implements legacy FirebaseService', () => {
         const deleteFake = fake();
         const myService: FirebaseService = {
           app: getFakeApp(),
@@ -226,6 +227,29 @@ describe('Provider', () => {
 
         expect(deleteFake).to.have.been.called;
       });
+
+      it('calls delete() on the service instance that implements next FirebaseService', () => {
+        const deleteFake = fake();
+        const myService: _FirebaseService = {
+          app: getFakeApp(),
+          _delete: deleteFake
+        };
+
+        // provide factory and create a service instance
+        provider.setComponent(
+          getFakeComponent(
+            'test',
+            () => myService,
+            false,
+            InstantiationMode.EAGER
+          )
+        );
+
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        provider.delete();
+
+        expect(deleteFake).to.have.been.called;
+      });
     });
 
     describe('clearCache()', () => {

packages/component/src/provider.ts

@@ -170,12 +170,16 @@ export class Provider<T extends Name> {
   async delete(): Promise<void> {
     const services = Array.from(this.instances.values());
 
-    await Promise.all(
-      services
-        .filter(service => 'INTERNAL' in service)
+    await Promise.all([
+      ...services
+        .filter(service => 'INTERNAL' in service) // legacy services
         // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        .map(service => (service as any).INTERNAL!.delete())
-    );
+        .map(service => (service as any).INTERNAL!.delete()),
+      ...services
+        .filter(service => '_delete' in service) // modularized services
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        .map(service => (service as any)._delete())
+    ]);
   }
 
   isComponentSet(): boolean {

packages/firestore/exp/src/api/components.ts

@@ -23,7 +23,7 @@ import {
   OfflineComponentProvider,
   OnlineComponentProvider
 } from '../../../src/core/component_provider';
-import {handleUserChange, LocalStore} from '../../../src/local/local_store';
+import { handleUserChange, LocalStore } from '../../../src/local/local_store';
 import { Deferred } from '../../../src/util/promise';
 import { logDebug } from '../../../src/util/log';
 import { SyncEngine } from '../../../src/core/sync_engine';
@@ -71,7 +71,7 @@ export async function setOfflineComponentProvider(
   // When a user calls clearPersistence() in one client, all other clients
   // need to be terminated to allow the delete to succeed.
   offlineComponentProvider.persistence.setDatabaseDeletedListener(() =>
-    firestore.delete()
+    firestore._delete()
   );
   offlineDeferred.resolve(offlineComponentProvider);
 }

packages/firestore/exp/src/api/database.ts

@@ -69,7 +69,8 @@ const LOG_TAG = 'Firestore';
  * The root reference to the Firestore database and the entry point for the
  * tree-shakeable SDK.
  */
-export class Firestore extends LiteFirestore
+export class Firestore
+  extends LiteFirestore
   implements firestore.FirebaseFirestore, _FirebaseService {
   readonly _queue = new AsyncQueue();
   readonly _persistenceKey: string;
@@ -320,7 +321,7 @@ export function terminate(
 ): Promise<void> {
   _removeServiceInstance(firestore.app, 'firestore-exp');
   const firestoreImpl = cast(firestore, Firestore);
-  return firestoreImpl.delete();
+  return firestoreImpl._delete();
 }
 
 function verifyNotInitialized(firestore: Firestore): void {

packages/firestore/lite/src/api/database.ts

@@ -95,7 +95,7 @@ export class Firestore
     return new DatabaseId(app.options.projectId!);
   }
 
-  delete(): Promise<void> {
+  _delete(): Promise<void> {
     if (!this._terminateTask) {
       this._terminateTask = this._terminate();
     }
@@ -117,7 +117,7 @@ export class Firestore
   // TODO(firestoreexp): `deleteApp()` should call the delete method above,
   // but it still calls INTERNAL.delete().
   INTERNAL = {
-    delete: () => this.delete()
+    delete: () => this._delete()
   };
 }
 
@@ -142,5 +142,5 @@ export function terminate(
 ): Promise<void> {
   _removeServiceInstance(firestore.app, 'firestore/lite');
   const firestoreClient = cast(firestore, Firestore);
-  return firestoreClient.delete();
+  return firestoreClient._delete();
 }