fix: Removing FirebaseServiceInterface and FirebaseServiceInternalsInterface (#1128)

* fix: Removing FirebaseService and FirebaseServiceInterface internal APIs

* chore: Added unit tests for service caching behavior

* fix: Using equal instead of deep.equal for reference tests

Author: hiranya911

src/auth/auth.ts

@@ -25,7 +25,6 @@ import {
   AbstractAuthRequestHandler, AuthRequestHandler, TenantAwareAuthRequestHandler, useEmulator,
 } from './auth-api-request';
 import { AuthClientErrorCode, FirebaseAuthError, ErrorInfo } from '../utils/error';
-import { FirebaseServiceInterface, FirebaseServiceInternalsInterface } from '../firebase-service';
 import * as utils from '../utils/index';
 import * as validator from '../utils/validator';
 import { auth } from './index';
@@ -59,22 +58,6 @@ import BaseAuthInterface = auth.BaseAuth;
 import AuthInterface = auth.Auth;
 import TenantAwareAuthInterface = auth.TenantAwareAuth;
 
-/**
- * Internals of an Auth instance.
- */
-class AuthInternals implements FirebaseServiceInternalsInterface {
-  /**
-   * Deletes the service and its associated resources.
-   *
-   * @return {Promise<()>} An empty Promise that will be fulfilled when the service is deleted.
-   */
-  public delete(): Promise<void> {
-    // There are no resources to clean up
-    return Promise.resolve(undefined);
-  }
-}
-
-
 /**
  * Base Auth class. Mainly used for user management APIs.
  */
@@ -820,10 +803,8 @@ export class TenantAwareAuth
  * Auth service bound to the provided app.
  * An Auth instance can have multiple tenants.
  */
-export class Auth extends BaseAuth<AuthRequestHandler>
-  implements FirebaseServiceInterface, AuthInterface {
+export class Auth extends BaseAuth<AuthRequestHandler> implements AuthInterface {
 
-  public INTERNAL: AuthInternals = new AuthInternals();
   private readonly tenantManager_: TenantManager;
   private readonly app_: FirebaseApp;
 

src/database/database-internal.ts

@@ -19,7 +19,6 @@ import * as path from 'path';
 
 import { FirebaseApp } from '../firebase-app';
 import { FirebaseDatabaseError, AppErrorCodes, FirebaseAppError } from '../utils/error';
-import { FirebaseServiceInterface, FirebaseServiceInternalsInterface } from '../firebase-service';
 import { Database as DatabaseImpl } from '@firebase/database';
 import { database } from './index';
 
@@ -29,35 +28,14 @@ import { getSdkVersion } from '../utils/index';
 
 import Database = database.Database;
 
-/**
- * Internals of a Database instance.
- */
-class DatabaseInternals implements FirebaseServiceInternalsInterface {
+export class DatabaseService {
+
+  private readonly appInternal: FirebaseApp;
 
-  public databases: {
+  private databases: {
     [dbUrl: string]: Database;
   } = {};
 
-  /**
-   * Deletes the service and its associated resources.
-   *
-   * @return {Promise<()>} An empty Promise that will be fulfilled when the service is deleted.
-   */
-  public delete(): Promise<void> {
-    for (const dbUrl of Object.keys(this.databases)) {
-      const db: DatabaseImpl = ((this.databases[dbUrl] as any) as DatabaseImpl);
-      db.INTERNAL.delete();
-    }
-    return Promise.resolve(undefined);
-  }
-}
-
-export class DatabaseService implements FirebaseServiceInterface {
-
-  public readonly INTERNAL: DatabaseInternals = new DatabaseInternals();
-
-  private readonly appInternal: FirebaseApp;
-
   constructor(app: FirebaseApp) {
     if (!validator.isNonNullObject(app) || !('options' in app)) {
       throw new FirebaseDatabaseError({
@@ -68,6 +46,20 @@ export class DatabaseService implements FirebaseServiceInterface {
     this.appInternal = app;
   }
 
+  /**
+   * @internal
+   */
+  public delete(): Promise<void> {
+    const promises = [];
+    for (const dbUrl of Object.keys(this.databases)) {
+      const db: DatabaseImpl = ((this.databases[dbUrl] as any) as DatabaseImpl);
+      promises.push(db.INTERNAL.delete());
+    }
+    return Promise.all(promises).then(() => {
+      this.databases = {};
+    });
+  }
+
   /**
    * Returns the app associated with this DatabaseService instance.
    *
@@ -86,7 +78,7 @@ export class DatabaseService implements FirebaseServiceInterface {
       });
     }
 
-    let db: Database = this.INTERNAL.databases[dbUrl];
+    let db: Database = this.databases[dbUrl];
     if (typeof db === 'undefined') {
       const rtdb = require('@firebase/database'); // eslint-disable-line @typescript-eslint/no-var-requires
       db = rtdb.initStandalone(this.appInternal, dbUrl, getSdkVersion()).instance;
@@ -102,7 +94,7 @@ export class DatabaseService implements FirebaseServiceInterface {
         return rulesClient.setRules(source);
       };
 
-      this.INTERNAL.databases[dbUrl] = db;
+      this.databases[dbUrl] = db;
     }
     return db;
   }

src/firebase-app.ts

@@ -19,8 +19,7 @@ import { AppOptions, app } from './firebase-namespace-api';
 import { credential, GoogleOAuthAccessToken } from './credential/index';
 import { getApplicationDefault } from './credential/credential-internal';
 import * as validator from './utils/validator';
-import { deepCopy, deepExtend } from './utils/deep-copy';
-import { FirebaseServiceInterface } from './firebase-service';
+import { deepCopy } from './utils/deep-copy';
 import { FirebaseNamespaceInternals } from './firebase-namespace';
 import { AppErrorCodes, FirebaseAppError } from './utils/error';
 
@@ -238,7 +237,7 @@ export class FirebaseApp implements app.App {
 
   private name_: string;
   private options_: AppOptions;
-  private services_: {[name: string]: FirebaseServiceInterface} = {};
+  private services_: {[name: string]: unknown} = {};
   private isDeleted_ = false;
 
   constructor(options: AppOptions, name: string, private firebaseInternals_: FirebaseNamespaceInternals) {
@@ -268,11 +267,6 @@ export class FirebaseApp implements app.App {
       );
     }
 
-    Object.keys(firebaseInternals_.serviceFactories).forEach((serviceName) => {
-      // Defer calling createService() until the service is accessed
-      (this as {[key: string]: any})[serviceName] = this.getService_.bind(this, serviceName);
-    });
-
     this.INTERNAL = new FirebaseAppInternals(credential);
   }
 
@@ -428,51 +422,24 @@ export class FirebaseApp implements app.App {
     this.INTERNAL.delete();
 
     return Promise.all(Object.keys(this.services_).map((serviceName) => {
-      return this.services_[serviceName].INTERNAL.delete();
+      const service = this.services_[serviceName];
+      if (isStateful(service)) {
+        return service.delete();
+      }
+      return Promise.resolve();
     })).then(() => {
       this.services_ = {};
       this.isDeleted_ = true;
     });
   }
 
-  private ensureService_<T extends FirebaseServiceInterface>(serviceName: string, initializer: () => T): T {
-    this.checkDestroyed_();
-
-    let service: T;
-    if (serviceName in this.services_) {
-      service = this.services_[serviceName] as T;
-    } else {
-      service = initializer();
-      this.services_[serviceName] = service;
-    }
-    return service;
-  }
-
-  /**
-   * Returns the service instance associated with this FirebaseApp instance (creating it on demand
-   * if needed). This is used for looking up monkeypatched service instances.
-   *
-   * @param serviceName The name of the service instance to return.
-   * @return The service instance with the provided name.
-   */
-  private getService_(serviceName: string): FirebaseServiceInterface {
+  private ensureService_<T>(serviceName: string, initializer: () => T): T {
     this.checkDestroyed_();
-
     if (!(serviceName in this.services_)) {
-      this.services_[serviceName] = this.firebaseInternals_.serviceFactories[serviceName](
-        this,
-        this.extendApp_.bind(this),
-      );
+      this.services_[serviceName] = initializer();
     }
 
-    return this.services_[serviceName];
-  }
-
-  /**
-   * Callback function used to extend an App instance at the time of service instance creation.
-   */
-  private extendApp_(props: {[prop: string]: any}): void {
-    deepExtend(this, props);
+    return this.services_[serviceName] as T;
   }
 
   /**
@@ -487,3 +454,11 @@ export class FirebaseApp implements app.App {
     }
   }
 }
+
+interface StatefulFirebaseService {
+  delete(): Promise<void>;
+}
+
+function isStateful(service: any): service is StatefulFirebaseService {
+  return typeof service.delete === 'function';
+}

src/firebase-namespace.ts

@@ -17,11 +17,9 @@
 
 import fs = require('fs');
 
-import { deepExtend } from './utils/deep-copy';
 import { AppErrorCodes, FirebaseAppError } from './utils/error';
 import { AppOptions, app } from './firebase-namespace-api';
-import { AppHook, FirebaseApp } from './firebase-app';
-import { FirebaseServiceFactory, FirebaseServiceInterface } from './firebase-service';
+import { FirebaseApp } from './firebase-app';
 import { cert, refreshToken, applicationDefault } from './credential/credential';
 import { getApplicationDefault } from './credential/credential-internal';
 
@@ -69,11 +67,8 @@ export interface FirebaseServiceNamespace <T> {
  * Internals of a FirebaseNamespace instance.
  */
 export class FirebaseNamespaceInternals {
-  public serviceFactories: {[serviceName: string]: FirebaseServiceFactory} = {};
 
   private apps_: {[appName: string]: App} = {};
-  private appHooks_: {[service: string]: AppHook} = {};
-
   constructor(public firebase_: {[key: string]: any}) {}
 
   /**
@@ -118,11 +113,7 @@ export class FirebaseNamespaceInternals {
     }
 
     const app = new FirebaseApp(options, appName, this);
-
     this.apps_[appName] = app;
-
-    this.callAppHooks_(app, 'create');
-
     return app;
   }
 
@@ -170,80 +161,7 @@ export class FirebaseNamespaceInternals {
     }
 
     const appToRemove = this.app(appName);
-    this.callAppHooks_(appToRemove, 'delete');
-    delete this.apps_[appName];
-  }
-
-  /**
-   * Registers a new service on this Firebase namespace.
-   *
-   * @param serviceName The name of the Firebase service to register.
-   * @param createService A factory method to generate an instance of the Firebase service.
-   * @param serviceProperties Optional properties to extend this Firebase namespace with.
-   * @param appHook Optional callback that handles app-related events like app creation and deletion.
-   * @return The Firebase service's namespace.
-   */
-  public registerService(
-    serviceName: string,
-    createService: FirebaseServiceFactory,
-    serviceProperties?: object,
-    appHook?: AppHook): FirebaseServiceNamespace<FirebaseServiceInterface> {
-    let errorMessage;
-    if (typeof serviceName === 'undefined') {
-      errorMessage = 'No service name provided. Service name must be a non-empty string.';
-    } else if (typeof serviceName !== 'string' || serviceName === '') {
-      errorMessage = `Invalid service name "${serviceName}" provided. Service name must be a non-empty string.`;
-    } else if (serviceName in this.serviceFactories) {
-      errorMessage = `Firebase service named "${serviceName}" has already been registered.`;
-    }
-
-    if (typeof errorMessage !== 'undefined') {
-      throw new FirebaseAppError(
-        AppErrorCodes.INTERNAL_ERROR,
-        `INTERNAL ASSERT FAILED: ${errorMessage}`,
-      );
-    }
-
-    this.serviceFactories[serviceName] = createService;
-    if (appHook) {
-      this.appHooks_[serviceName] = appHook;
-    }
-
-    // The service namespace is an accessor function which takes a FirebaseApp instance
-    // or uses the default app if no FirebaseApp instance is provided
-    const serviceNamespace: FirebaseServiceNamespace<FirebaseServiceInterface> = (appArg?: App) => {
-      if (typeof appArg === 'undefined') {
-        appArg = this.app();
-      }
-
-      // Forward service instance lookup to the FirebaseApp
-      return (appArg as any)[serviceName]();
-    };
-
-    // ... and a container for service-level properties.
-    if (serviceProperties !== undefined) {
-      deepExtend(serviceNamespace, serviceProperties);
-    }
-
-    // Monkey-patch the service namespace onto the Firebase namespace
-    this.firebase_[serviceName] = serviceNamespace;
-
-    return serviceNamespace;
-  }
-
-  /**
-   * Calls the app hooks corresponding to the provided event name for each service within the
-   * provided App instance.
-   *
-   * @param app The App instance whose app hooks to call.
-   * @param eventName The event name representing which app hooks to call.
-   */
-  private callAppHooks_(app: App, eventName: string): void {
-    Object.keys(this.serviceFactories).forEach((serviceName) => {
-      if (this.appHooks_[serviceName]) {
-        this.appHooks_[serviceName](eventName, app);
-      }
-    });
+    delete this.apps_[appToRemove.name];
   }
 
   /**