Allow for lazy registration of app components #15

jshcrowthe · 2017-05-18T03:36:22Z

Fixes #14

Live demo available here:
http://jsbin.com/zozuqeh/edit?js,console

…ntiation

mbleigh · 2017-05-18T16:14:14Z

Well that's a surprisingly small change. I think the concern previously was cross-product hooks, particularly with Auth. Have you tried this out with loading the Database first, then Auth later?

So 1) load DB, read some data that doesn't require auth 2) load auth lazily, load some data from the DB that does require auth.

jshcrowthe · 2017-05-18T18:33:55Z

@mbleigh So the cross-product hooks that you mentioned are all taken care of. However I did update my JSBin to demo your use case and discovered that the stub methods that FirebaseApp creates for the auth, needs to forward the listeners created on to the auth module if it is lazily instantiated. Should hopefully have that working in just a bit.

AFAICT none of the other products have this issue though as they all call into getToken at the time of the call whereas database needs to register token listeners.

justweb1 · 2017-05-21T20:41:49Z

@jshcrowthe I was trying to figure out a way to auto inititializeApp when hosted on Firebase. I was trying this through Polymerfire but the issue @mbleigh brought up was lazy loading the elements. Would it be appropriate to add a test to your PR to see if '/__/firebase/init.js' is present and then import it and run initializeApp automatically?

mbleigh

I think my main question/concern at this point is that there is now auth-specific code hanging out in FirebaseApp. Is there any way to expose hooks in FirebaseApp that are instead consumed by Firebase Auth in such a way that the same behavior happens Right now this feels like an awesome proof-of-concept but not necessarily a stable foundation.

jshcrowthe · 2017-05-22T19:39:27Z

The reason why I followed this approach is largely based on how modules are currently required to register with Firebase App.

We are currently stubbing some auth-specific code that is required by certain modules (i.e. storage, database) in the event that auth does not exist (see https://github.com/firebase/firebase-js-sdk/blob/master/src/app/firebase_app.ts#L433-L448). This implies a dependency of each of these components upon those auth methods. As our source is currently spread across module implementations (some rely on imports and our webpack compilation, others rely on the firebase and firebase.INTERNAL namespaces), we have some workarounds in place to help bridge this gap (namely the firebase.INTERNAL utilities).

Once we have access to the Auth source in this repo, a refactoring of this code (i.e. removal of this code) and the associated database/storage code, should happen that would extract this specific module into a standalone piece. This piece would stub the API and provide each component (in this case auth) the ability to control how this functionality is "upgraded." That said, with how we currently distribute to CDNs, the "stubbing" module would be included with firebase-app.js as this code is shared across webpack entry points.

I will add a comment to explain the need for this and a "todo" to remove it once able?

…ase_app.ts

bojeil-google · 2017-05-22T23:11:46Z

src/app/firebase_app.ts

-  private services_: {[name: string]:
-                          {[instance: string]: FirebaseService}} = {};
-  public INTERNAL: FirebaseAppInternals;
+  private _registeredServices: {


This doesn't seem to be used. Also, you seem to be prefixing private variables and methods with an underscore. This is not the convention here. I think we should stick to one format. In general the typescript convention is not to use leading or lagging underscores for private variables/methods.

Good catch I'll remove this. It was an artifact of an old implementation.

bojeil-google · 2017-05-22T23:43:31Z

src/app/firebase_app.ts

   */
-  private getService(name: string, instanceString?: string): FirebaseService


What happened to this? By not tracking this, I think you will return the same default instance for storage. I think you could have multiple instances of storage with different buckets.

I see what you mean! I'm surprised we don't have a test for this. I'll revert the majority of this change (it didn't seem used so it was a clean up thing) and add a test to make sure we don't break that going forward! I will need to keep the removal of the private keyword as we need to call this outside of this classes implementation details.

bojeil-google · 2017-05-22T23:46:17Z

src/app/firebase_app.ts

    if (appHook) {
      appHooks[name] = appHook;
+      getApps().forEach(app => {
+        appHook('create', app);


This will trigger every time a service is registered to the same app. Even when it was previously already triggered. Not sure what the implications are for other services listening to this.

The two functions that currently will trigger the create hook for services are initializeApp and registerService. Though they are similar they are slightly different and actual have no overlap in terms of when appHooks are fired.

initializeApp attempts to run appHooks for each factory that exists at the time of app initialization.

registerService runs only the new appHook (if applicable) on all existing instances of FirebaseApp it then also registers itself as one of the available appHooks so that future initializeApp calls will trigger the hook.

The create hook will be run once, and only once, per instance of Firebase App. Because one can't register multiple services of the same name, attempting to register multiple appHooks under the same name, and trigger a duplicate function call, should fail with an error long before the call is ever attempted.

mbleigh

I AM EXCITE!

However, folks more familiar with the SDK should approve before this gets merged. Let's make sure we do a minor version bump when this gets released.

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

jshcrowthe · 2017-05-23T22:05:38Z

@bojeil-google I have addressed some of your concerns and added some tests to verify that functionality stays functioning (turns out there was an alternate way to do it that I brought into compliance with the API).

jshcrowthe · 2017-05-24T00:55:56Z

@justweb1 I don't know if this PR is the right place to add that type of functionality. That said, I think if we were to do something explicit (i.e. something like allowing people to call firebase.initializeApp w/o any args, or a separate function altogether) for this that'd work.

However I can also see this type of functionality being a nice thing for an abstraction layer like Polymerfire, angularfire, etc.

Thoughts?

justweb1 · 2017-05-24T05:19:21Z

The thought was originally for it to be built into Polymerfire and I had even started work on it. After talking with @mbleigh, it seems to make more sense to run it directly in the sdk. One of the main issues I was having was due to the fact I needed lazy loading. Then, when it boils down to it placing a check and calling the init.js directly from the sdk is a much better option and will require way less code. This will also simplify the initialization and improve user experience.

jshcrowthe · 2017-05-24T14:51:06Z

@justweb1 let's open a separate issue for this and continue discussion there.

sphippen · 2017-05-24T18:23:07Z

src/app/firebase_app.ts

+     * otherwise, return a proxied reference to the same service
+     */
+    factories[name] = allowMultipleInstances ? createService :
+      (...args) => createService.apply(this, args);


Seems like this ternary statement isn't really doing anything? Before, the if statement was always sending DEFAULT_ENTRY_NAME instead of the passed instanceString if !allowMultipleInstances, but since you handle the multiple instance name thing further down now (inside the function assigned to FirebaseAppImpl.prototype[name]), you should just be able to do

factories[name] = createService;

and have it work, right?

Great point, I think I had meant to do this, but never actually did it. Fixed!

sphippen

LGTM

davideast

If you make @sphippen happy, then I'm happy. Let's get this in 💥

bojeil-google · 2017-05-25T07:06:09Z

src/app/firebase_app.ts

+    FirebaseAppImpl.prototype[name] = function(...args) {
+      const serviceFxn = this._getService.bind(this, name);
+      return serviceFxn.apply(this, allowMultipleInstances ? args : []);
+    }


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.

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.

bojeil-google · 2017-05-25T07:21:59Z

tests/app/unit/firebase_app.test.ts

+    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**


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.

Gotcha, thanks!

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.

jshcrowthe · 2017-05-25T16:11:26Z

@bojeil-google I have responded to your last two comments. Thanks sir!

We should be good to go. I'll get this merged.

bojeil-google · 2017-05-25T17:31:25Z

Looks good. Thanks!

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

jshcrowthe added 3 commits May 17, 2017 19:57

WIP: add lazy module instantiation

718c99c

feat(app): allow for lazy loading of services

1413770

test(app): add integration test for webpack/browserify for lazy insta…

f26cfc5

…ntiation

jshcrowthe requested review from mbleigh and davideast May 18, 2017 03:36

google-oss-bot added the needs-triage label May 18, 2017

jshcrowthe added api: core and removed needs-triage labels May 18, 2017

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

3a38558

jshcrowthe requested review from sphippen and bojeil-google May 19, 2017 17:17

mbleigh reviewed May 22, 2017

View reviewed changes

docs(app): adding doc explaining Firebase Auth specific code in fireb…

47d618a

…ase_app.ts

bojeil-google reviewed May 23, 2017

View reviewed changes

mbleigh approved these changes May 23, 2017

View reviewed changes

jshcrowthe added 2 commits May 23, 2017 14:51

fix(app): revert code refactor to _getService function

9baf65f

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

99820ef

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

justweb1 mentioned this pull request May 24, 2017

Allow SDK to auto initializeApp when hosted on Firebase #23

Closed

sphippen reviewed May 24, 2017

View reviewed changes

refactor(app): remove unneeded ternary

3e211b3

sphippen approved these changes May 24, 2017

View reviewed changes

davideast reviewed May 24, 2017

View reviewed changes

jshcrowthe self-assigned this May 24, 2017

bojeil-google reviewed May 25, 2017

View reviewed changes

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

c75fb59

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.

jshcrowthe force-pushed the lazy-register branch from ba477fa to c75fb59 Compare May 25, 2017 16:05

jshcrowthe merged commit bd24f71 into master May 25, 2017

jshcrowthe deleted the lazy-register branch May 25, 2017 16:12

renovate-tests mentioned this pull request Jul 7, 2017

Update dependency firebase to v4 renovate-tests/discover-package-files#18

Open

firebase locked and limited conversation to collaborators Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for lazy registration of app components #15

Allow for lazy registration of app components #15

jshcrowthe commented May 18, 2017 •

edited

mbleigh commented May 18, 2017

jshcrowthe commented May 18, 2017 •

edited

justweb1 commented May 21, 2017 •

edited

mbleigh left a comment

jshcrowthe commented May 22, 2017 •

edited

bojeil-google May 22, 2017

jshcrowthe May 23, 2017

bojeil-google May 22, 2017

jshcrowthe May 23, 2017 •

edited

bojeil-google May 22, 2017

jshcrowthe May 23, 2017 •

edited

mbleigh left a comment •

edited by jshcrowthe

jshcrowthe commented May 23, 2017

jshcrowthe commented May 24, 2017

justweb1 commented May 24, 2017

jshcrowthe commented May 24, 2017

sphippen May 24, 2017

jshcrowthe May 24, 2017

sphippen left a comment

davideast left a comment

bojeil-google May 25, 2017 •

edited by jshcrowthe

jshcrowthe May 25, 2017

bojeil-google May 25, 2017 •

edited by jshcrowthe

jshcrowthe May 25, 2017

jshcrowthe commented May 25, 2017

bojeil-google commented May 25, 2017

		*/
		private getService(name: string, instanceString?: string): FirebaseService

Allow for lazy registration of app components #15

Allow for lazy registration of app components #15

Conversation

jshcrowthe commented May 18, 2017 • edited

mbleigh commented May 18, 2017

jshcrowthe commented May 18, 2017 • edited

justweb1 commented May 21, 2017 • edited

mbleigh left a comment

Choose a reason for hiding this comment

jshcrowthe commented May 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe May 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe May 23, 2017 • edited

Choose a reason for hiding this comment

mbleigh left a comment • edited by jshcrowthe

Choose a reason for hiding this comment

jshcrowthe commented May 23, 2017

jshcrowthe commented May 24, 2017

justweb1 commented May 24, 2017

jshcrowthe commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphippen left a comment

Choose a reason for hiding this comment

davideast left a comment

Choose a reason for hiding this comment

bojeil-google May 25, 2017 • edited by jshcrowthe

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojeil-google May 25, 2017 • edited by jshcrowthe

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe commented May 25, 2017

bojeil-google commented May 25, 2017

jshcrowthe commented May 18, 2017 •

edited

jshcrowthe commented May 18, 2017 •

edited

justweb1 commented May 21, 2017 •

edited

jshcrowthe commented May 22, 2017 •

edited

jshcrowthe May 23, 2017 •

edited

jshcrowthe May 23, 2017 •

edited

mbleigh left a comment •

edited by jshcrowthe

bojeil-google May 25, 2017 •

edited by jshcrowthe

bojeil-google May 25, 2017 •

edited by jshcrowthe