Merge a7df102 into 6a95ae1

.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/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,25 @@ 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 &&
+            this.component &&
+            // TODO: remove !== 'app' when modular SDKs become official
+            // People call app.delete() to trigger provider.delete() for all registered components, so
+            // we don't call delete() on legacy FirebaseApp to avoid getting into a loop.
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            (this.component.name as any) !== 'app'
+        ) // modular services
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        .map(service => (service as any).delete())
+    ]);
   }
 
   isComponentSet(): boolean {