delete services that implement the new FirebaseService interface

.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-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/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

@@ -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

@@ -320,7 +320,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();
 }

packages/firestore/scripts/run-tests.js

@@ -1,4 +1,4 @@
-"use strict";
+'use strict';
 /**
  * @license
  * Copyright 2020 Google LLC
@@ -14,4 +14,51 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
- */exports.__esModule=true;var yargs=require("yargs");var path_1=require("path");var child_process_promise_1=require("child-process-promise");var argv=yargs.options({main:{type:"string",demandOption:true},platform:{type:"string",default:"node"},emulator:{type:"boolean"},persistence:{type:"boolean"}}).argv;var nyc=path_1.resolve(__dirname,"../../../node_modules/.bin/nyc");var mocha=path_1.resolve(__dirname,"../../../node_modules/.bin/mocha");process.env.TS_NODE_CACHE="NO";process.env.TS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}';process.env.TEST_PLATFORM=argv.platform;var args=["--reporter","lcovonly",mocha,"--require","ts-node/register","--require",argv.main,"--config","../../config/mocharc.node.js"];if(argv.emulator){process.env.FIRESTORE_EMULATOR_PORT="8080";process.env.FIRESTORE_EMULATOR_PROJECT_ID="test-emulator"}if(argv.persistence){process.env.USE_MOCK_PERSISTENCE="YES";args.push("--require","test/util/node_persistence.ts")}args=args.concat(argv._);var childProcess=child_process_promise_1.spawn(nyc,args,{stdio:"inherit",cwd:process.cwd()}).childProcess;process.once("exit",(function(){return childProcess.kill()}));process.once("SIGINT",(function(){return childProcess.kill("SIGINT")}));process.once("SIGTERM",(function(){return childProcess.kill("SIGTERM")}));
+ */ exports.__esModule = true;
+var yargs = require('yargs');
+var path_1 = require('path');
+var child_process_promise_1 = require('child-process-promise');
+var argv = yargs.options({
+  main: { type: 'string', demandOption: true },
+  platform: { type: 'string', default: 'node' },
+  emulator: { type: 'boolean' },
+  persistence: { type: 'boolean' }
+}).argv;
+var nyc = path_1.resolve(__dirname, '../../../node_modules/.bin/nyc');
+var mocha = path_1.resolve(__dirname, '../../../node_modules/.bin/mocha');
+process.env.TS_NODE_CACHE = 'NO';
+process.env.TS_NODE_COMPILER_OPTIONS = '{"module":"commonjs"}';
+process.env.TEST_PLATFORM = argv.platform;
+var args = [
+  '--reporter',
+  'lcovonly',
+  mocha,
+  '--require',
+  'ts-node/register',
+  '--require',
+  argv.main,
+  '--config',
+  '../../config/mocharc.node.js'
+];
+if (argv.emulator) {
+  process.env.FIRESTORE_EMULATOR_PORT = '8080';
+  process.env.FIRESTORE_EMULATOR_PROJECT_ID = 'test-emulator';
+}
+if (argv.persistence) {
+  process.env.USE_MOCK_PERSISTENCE = 'YES';
+  args.push('--require', 'test/util/node_persistence.ts');
+}
+args = args.concat(argv._);
+var childProcess = child_process_promise_1.spawn(nyc, args, {
+  stdio: 'inherit',
+  cwd: process.cwd()
+}).childProcess;
+process.once('exit', function () {
+  return childProcess.kill();
+});
+process.once('SIGINT', function () {
+  return childProcess.kill('SIGINT');
+});
+process.once('SIGTERM', function () {
+  return childProcess.kill('SIGTERM');
+});