Skip to content

Allow for lazy registration of app components #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 25, 2017
Merged
2 changes: 1 addition & 1 deletion gulp/tasks/test.integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`;
Expand Down
150 changes: 83 additions & 67 deletions src/app/firebase_app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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 = [];
}
}

/**
Expand All @@ -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 &&
Expand Down Expand Up @@ -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;
}

Expand All @@ -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]();
};
Expand All @@ -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 : []);
}
Copy link
Contributor

@bojeil-google bojeil-google May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's late, maybe I'm misreading:
So this will patch in the service factory: app.storage(instanceString);
Will this pass instanceString to createService(app, extendApp, instanceString)?

This statement, as quoted from above, doesn't pass the instance string.

let service = this.firebase_.INTERNAL.factories[name](this, this.extendApp.bind(this));

If you update the test below where I commented to confirm this, then you are good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch on this. I have refactored to ensure that we are passing through the instanceIdentifier I must have missed it in adding code back.

In addition I have added an additional assertion to two of the tests below to ensure that we don't break this going forward.


return serviceNamespace;
}

Expand Down
21 changes: 21 additions & 0 deletions tests-integration/bundlers/browser/lazy-app.test.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
119 changes: 116 additions & 3 deletions tests/app/unit/firebase_app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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**
Copy link
Contributor

@bojeil-google bojeil-google May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't check that the two difference services are getting their identifier passed to the underlying service constructor.
For Storage, this is:

function factory(app: FirebaseApp, unused: any, opt_url?: string): Service {
  return new Service(app, new XhrIoPool(), opt_url);
}

opt_url needs to be passed through.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks!

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) {
Expand All @@ -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?
Expand Down
Loading