feat(app): Allow for lazy registration of app components (#15)

* WIP: add lazy module instantiation

* feat(app): allow for lazy loading of services

* test(app): add integration test for webpack/browserify for lazy instantiation

* fix(app): fix issue where auth listeners weren't being patched properly

* docs(app): adding doc explaining Firebase Auth specific code in firebase_app.ts

* fix(app): revert code refactor to _getService function

* test(app): add tests and fix issue with multiple instances of a service

There was an issue where we weren't properly limiting mulitple service instances. Added a test and
refactored the code to support that use case

* refactor(app): remove unneeded ternary

* fix(app): fix issue where instanceIdentifier wasn't being passed

Fixed an issue where instance identifiers weren't being passed to their creation factories. Added
some tests to validate that we don't accidentally remove that in the future.

Author: jshcrowthe

gulp/tasks/test.integration.js

@@ -48,7 +48,7 @@ function compileTypescript() {
 function compileWebpack() {
   return gulp.src('tests-integration/bundlers/**/*.test.js')
     .pipe(named())
-    .pipe(webpackStream())
+    .pipe(webpackStream({}, webpack))
     .pipe(rename(path => {
       const rawPath = path.basename.replace('.test', '');
       path.basename = `${rawPath}.webpack.test`;

src/app/firebase_app.ts

@@ -221,6 +221,10 @@ let LocalPromise = local.Promise as typeof Promise;
 
 const DEFAULT_ENTRY_NAME = '[DEFAULT]';
 
+// An array to capture listeners before the true auth functions
+// exist
+let tokenListeners = [];
+
 /**
  * Global context object for a collection of services using
  * a shared authentication state.
@@ -229,27 +233,19 @@ class FirebaseAppImpl implements FirebaseApp {
   private options_: FirebaseOptions;
   private name_: string;
   private isDeleted_ = false;
-  private services_: {[name: string]:
-                          {[instance: string]: FirebaseService}} = {};
-  public INTERNAL: FirebaseAppInternals;
+  private services_: {
+    [name: string]: {
+      [serviceName: string]: FirebaseService
+    }
+  } = {};
+
+  public INTERNAL;
 
   constructor(options: FirebaseOptions,
               name: string,
               private firebase_: FirebaseNamespace) {
     this.name_ = name;
     this.options_ = deepCopy<FirebaseOptions>(options);
-
-    Object.keys(firebase_.INTERNAL.factories).forEach((serviceName) => {
-      // Ignore virtual services
-      let factoryName = firebase_.INTERNAL.useAsService(this, serviceName);
-      if (factoryName === null) {
-        return;
-      }
-
-      // Defer calling createService until service is accessed.
-      let getService = this.getService.bind(this, factoryName);
-      patchProperty(this, serviceName, getService);
-    });
   }
 
   get name(): string {
@@ -286,34 +282,57 @@ class FirebaseAppImpl implements FirebaseApp {
   }
 
   /**
-   * Return the service instance associated with this app (creating it
-   * on demand).
+   * Return a service instance associated with this app (creating it
+   * on demand), identified by the passed instanceIdentifier.
+   * 
+   * NOTE: Currently storage is the only one that is leveraging this
+   * functionality. They invoke it by calling:
+   * 
+   * ```javascript
+   * firebase.app().storage('STORAGE BUCKET ID')
+   * ```
+   * 
+   * The service name is passed to this already
+   * @internal
    */
-  private getService(name: string, instanceString?: string): FirebaseService
-      |null {
+  _getService(name: string, instanceIdentifier: string = DEFAULT_ENTRY_NAME): FirebaseService {
     this.checkDestroyed_();
 
-    if (typeof this.services_[name] === 'undefined') {
+    if (!this.services_[name]) {
       this.services_[name] = {};
     }
 
-    let instanceSpecifier = instanceString || DEFAULT_ENTRY_NAME;
-    if (typeof this.services_[name]![instanceSpecifier] === 'undefined') {
-      let firebaseService = this.firebase_.INTERNAL.factories[name](
-          this, this.extendApp.bind(this), instanceString);
-      this.services_[name]![instanceSpecifier] = firebaseService;
-      return firebaseService;
-    } else {
-      return this.services_[name]![instanceSpecifier] as FirebaseService | null;
+    if (!this.services_[name][instanceIdentifier]) {
+      let service = this.firebase_.INTERNAL.factories[name](this, this.extendApp.bind(this), instanceIdentifier);
+      this.services_[name][instanceIdentifier] = service;
     }
+
+    return this.services_[name][instanceIdentifier];
   }
 
   /**
    * Callback function used to extend an App instance at the time
    * of service instance creation.
    */
   private extendApp(props: {[name: string]: any}): void {
-    deepExtend(this, props);
+    // Copy the object onto the FirebaseAppImpl prototype
+    deepExtend(FirebaseAppImpl.prototype, props);
+
+    /**
+     * If the app has overwritten the addAuthTokenListener stub, forward
+     * the active token listeners on to the true fxn.
+     * 
+     * TODO: This function is required due to our current module
+     * structure. Once we are able to rely strictly upon a single module
+     * implementation, this code should be refactored and Auth should 
+     * provide these stubs and the upgrade logic
+     */
+    if (props.INTERNAL && props.INTERNAL.addAuthTokenListener) {
+      tokenListeners.forEach(listener => {
+        this.INTERNAL.addAuthTokenListener(listener);
+      });
+      tokenListeners = [];
+    }
   }
 
   /**
@@ -327,6 +346,19 @@ class FirebaseAppImpl implements FirebaseApp {
   }
 };
 
+FirebaseAppImpl.prototype.INTERNAL = {
+  'getUid': () => null,
+  'getToken': () => LocalPromise.resolve(null),
+  'addAuthTokenListener': (callback: (token: string|null) => void) => {
+    tokenListeners.push(callback);
+    // Make sure callback is called, asynchronously, in the absence of the auth module
+    setTimeout(() => callback(null), 0);
+  },
+  'removeAuthTokenListener': (callback) => {
+    tokenListeners = tokenListeners.filter(listener => listener !== callback);
+  },
+}
+
 // Prevent dead-code elimination of these methods w/o invalid property
 // copying.
 FirebaseAppImpl.prototype.name &&
@@ -425,27 +457,12 @@ export function createFirebaseNamespace(): FirebaseNamespace {
     if (apps_[name!] !== undefined) {
       error('duplicate-app', {'name': name});
     }
-    let app = new FirebaseAppImpl(options, name!,
-                                  ((namespace as any) as FirebaseNamespace));
+
+    let app = new FirebaseAppImpl(options, name!, namespace as FirebaseNamespace);
+
     apps_[name!] = app;
     callAppHooks(app, 'create');
 
-    // Ensure that getUid, getToken, addAuthListener and removeAuthListener
-    // have a default implementation if no service has patched the App
-    // (i.e., Auth is not present).
-    if (app.INTERNAL == undefined || app.INTERNAL.getToken == undefined) {
-      deepExtend(app, {
-        INTERNAL: {
-          'getUid': () => null,
-          'getToken': () => LocalPromise.resolve(null),
-          'addAuthTokenListener': (callback: (token: string|null) => void) => {
-            // Make sure callback is called, asynchronously, in the absence of the auth module
-            setTimeout(() => callback(null), 0);
-          },
-          'removeAuthTokenListener': () => { /*_*/ },
-        }
-      });
-    }
     return app;
   }
 
@@ -471,39 +488,32 @@ export function createFirebaseNamespace(): FirebaseNamespace {
       appHook?: AppHook,
       allowMultipleInstances?: boolean):
       FirebaseServiceNamespace<FirebaseService> {
+    // Cannot re-register a service that already exists
     if (factories[name]) {
       error('duplicate-service', {'name': name});
     }
-    if (!!allowMultipleInstances) {
-      // Check if the service allows multiple instances per app
-      factories[name] = createService;
-    } else {
-      // If not, always return the same instance when a service is instantiated
-      // with an instanceString different than the default.
-      factories[name] =
-          (app: FirebaseApp, extendApp?: (props: {[prop: string]: any}) => void,
-           instanceString?: string) => {
-            // If a new instance is requested for a service that does not allow
-            // multiple instances, return the default instance
-            return createService(app, extendApp, DEFAULT_ENTRY_NAME);
-          };
-    }
+
+    // Capture the service factory for later service instantiation
+    factories[name] = createService;
+    
+    // Capture the appHook, if passed
     if (appHook) {
       appHooks[name] = appHook;
-    }
 
-    let serviceNamespace: FirebaseServiceNamespace<FirebaseService>;
+      // Run the **new** app hook on all existing apps
+      getApps().forEach(app => {
+        appHook('create', app);
+      });
+    }
 
     // The Service namespace is an accessor function ...
-    serviceNamespace = (appArg?: FirebaseApp) => {
-      if (appArg === undefined) {
-        appArg = app();
-      }
+    const serviceNamespace = (appArg: FirebaseApp = app()) => {
       if (typeof(appArg as any)[name] !== 'function') {
         // Invalid argument.
         // This happens in the following case: firebase.storage('gs:/')
         error('invalid-app-argument', {'name': name});
       }
+
       // Forward service instance lookup to the FirebaseApp.
       return (appArg as any)[name]();
     };
@@ -516,6 +526,12 @@ export function createFirebaseNamespace(): FirebaseNamespace {
     // Monkey-patch the serviceNamespace onto the firebase namespace
     (namespace as any)[name] = serviceNamespace;
 
+    // Patch the FirebaseAppImpl prototype
+    FirebaseAppImpl.prototype[name] = function(...args) {
+      const serviceFxn = this._getService.bind(this, name);
+      return serviceFxn.apply(this, allowMultipleInstances ? args : []);
+    }
+
     return serviceNamespace;
   }
 

tests-integration/bundlers/browser/lazy-app.test.js

@@ -0,0 +1,21 @@
+var assert = require('chai').assert;
+// Partial inclusion is a browser-only feature
+var firebase = require('../../../dist/package/app');
+var helper = require('./test-helper.js');
+
+describe("Lazy Firebase App (" + helper.getPackagerName() + ")", function() {
+  it("firebase namespace", function() {
+    assert.isDefined(firebase);
+  });
+
+  it("SDK_VERSION", function() {
+    assert.isDefined(firebase.SDK_VERSION);
+  });
+
+  it('Should allow for lazy component init', function() {
+    assert.isUndefined(firebase.database);
+    firebase.initializeApp({});
+    require('../../../dist/package/database');
+    assert.isDefined(firebase.database);
+  });
+});

tests/app/unit/firebase_app.test.ts

@@ -166,6 +166,121 @@ describe("Firebase App Class", () => {
     assert.equal(registrations, 2);
   });
 
+  it("Can lazy load a service", () => {
+    let registrations = 0;
+
+    const app1 = firebase.initializeApp({});
+    assert.isUndefined((app1 as any).lazyService);
+
+    firebase.INTERNAL.registerService('lazyService', (app: FirebaseApp) => {
+      registrations += 1;
+      return new TestService(app);
+    });
+
+    assert.isDefined((app1 as any).lazyService);    
+
+    // Initial service registration happens on first invocation
+    assert.equal(registrations, 0);
+
+    // Verify service has been registered
+    (firebase as any).lazyService();
+    assert.equal(registrations, 1);
+
+    // Service should only be created once
+    (firebase as any).lazyService();
+    assert.equal(registrations, 1);
+
+    // Service should only be created once... regardless of how you invoke the function
+    (firebase as any).lazyService(app1);
+    assert.equal(registrations, 1);
+
+    // Service should already be defined for the second app
+    const app2 = firebase.initializeApp({}, 'second');
+    assert.isDefined((app1 as any).lazyService);
+    
+    // Service still should not have registered for the second app
+    assert.equal(registrations, 1);
+
+    // Service should initialize once called
+    (app2 as any).lazyService();
+    assert.equal(registrations, 2);
+  });
+
+  it("Can lazy register App Hook", (done) => {
+    let events = ['create', 'delete'];
+    let hookEvents = 0;
+    const app = firebase.initializeApp({});
+    firebase.INTERNAL.registerService(
+      'lazyServiceWithHook',
+      (app: FirebaseApp) => {
+        return new TestService(app);
+      },
+      undefined,
+      (event: string, app: FirebaseApp) => {
+        assert.equal(event, events[hookEvents]);
+        hookEvents += 1;
+        if (hookEvents === events.length) {
+          done();
+        }
+      });
+    // Ensure the hook is called synchronously
+    assert.equal(hookEvents, 1);
+    app.delete();
+  });
+
+  it('Can register multiple instances of some services', () => {
+    // Register Multi Instance Service
+    firebase.INTERNAL.registerService(
+      'multiInstance',
+      (...args) => {
+        const [app,,instanceIdentifier] = args;
+        return new TestService(app, instanceIdentifier);
+      },
+      null,
+      null,
+      true
+    );
+    firebase.initializeApp({});
+
+    // Capture a given service ref
+    const service = (firebase.app() as any).multiInstance();
+    assert.strictEqual(service, (firebase.app() as any).multiInstance());
+
+    // Capture a custom instance service ref
+    const serviceIdentifier = 'custom instance identifier';
+    const service2 = (firebase.app() as any).multiInstance(serviceIdentifier);
+    assert.strictEqual(service2, (firebase.app() as any).multiInstance(serviceIdentifier));
+
+    // Ensure that the two services **are not equal**
+    assert.notStrictEqual(service.instanceIdentifier, service2.instanceIdentifier, '`instanceIdentifier` is not being set correctly');
+    assert.notStrictEqual(service, service2);
+    assert.notStrictEqual((firebase.app() as any).multiInstance(), (firebase.app() as any).multiInstance(serviceIdentifier));
+  });
+
+  it(`Should return the same instance of a service if a service doesn't support multi instance`, () => {
+    // Register Multi Instance Service
+    firebase.INTERNAL.registerService(
+      'singleInstance',
+      (...args) => {
+        const [app,,instanceIdentifier] = args;
+        return new TestService(app, instanceIdentifier)
+      },
+      null,
+      null,
+      false // <-- multi instance flag
+    );
+    firebase.initializeApp({});
+
+    // Capture a given service ref
+    const serviceIdentifier = 'custom instance identifier';
+    const service = (firebase.app() as any).singleInstance();
+    const service2 = (firebase.app() as any).singleInstance(serviceIdentifier);
+
+    // Ensure that the two services **are equal**
+    assert.strictEqual(service.instanceIdentifier, service2.instanceIdentifier, '`instanceIdentifier` is not being set correctly');
+    assert.strictEqual(service, service2);
+  });
+
   describe("Check for bad app names", () => {
     let tests = ["", 123, false, null];
     for (let data of tests) {
@@ -179,9 +294,7 @@ describe("Firebase App Class", () => {
 });
 
 class TestService implements FirebaseService {
-  constructor(private app_: FirebaseApp) {
-    // empty
-  }
+  constructor(private app_: FirebaseApp, public instanceIdentifier?: string) {}
 
   // TODO(koss): Shouldn't this just be an added method on
   // the service instance?