Migrate firestore to component framework

[Feiyang1]

Context

go/firebase-js-platform
#2316 implements the proposal.

Changes

This PR changes @firebase/firestore to use the component platform implemented in #2316

Dependencies

#2316
#2317

[mikelehen]

LGTM but I'll let @wilhuff approve since he reviewed the previous iterations.

[mikelehen]

Oh, also:

1. The travis build is failing. :-)
2. We need to manually test this since we have no automated auth / Firestore integration tests unfortunately. Let me know if you need help with this.

[wilhuff]

undefined here is wrong. We should be getting the auth instance associated with the current Firebase App.

Most of the reason why people use Firestore with multiple apps is to be able to associate multiple different login credentials with different logical sessions. Imagine if you were implementing Gmail's mobile app with Firestore: each identity (and its associated mailboxes from Firestore) should be a separate App, making it possible to simultaneously get new mail notifications from all currently signed-in accounts.

Tying this unconditionally to the default app breaks secondary sign-in.

Indeed I'm not sure why Provider would want to expose this kind of functionality to callers at all. Provider should give the instance scoped to the current app. If the SDK exposed by the provider is a singleton that should be that SDK's problem in how it registers its component.

[Feiyang1]

I think there is a misunderstanding here. The provider instances are indeed scoped to the current app (Not necessarily the default app), so it should work as we expect.

Some SDKs allow multiple instances per app, e.g. Storage and Database. The first parameter of getImmediate() let you provide the name for the specific service instance scoped to the same app.

Firestore doesn't support multiple instances per app, so we pass 'undefined'. Actually you can pass any string, but the provider will only create and cache a single instance of Firestore, because we set multipleInstances to false when registering Firestore component.

[mikelehen]

I may be confused but I think undefined is some sort of identifier string (https://github.com/firebase/firebase-js-sdk/blob/fei-component-firestore/packages/component/src/provider.ts#L57) and isn't too significant (though I'm not sure what it's for)...

Given that getImmediate() allows for an options object already it seems like moving identifier into the options object would be much cleaner. i.e.:

// here we would just do:
this.auth = authProvider.getImmediate({ optional: true });

// Elsewhere that identifier is used you would do:
foo = fooProvider.getImmediate({ identifier: 'gobbly gook' });

Passing literal values like true, false, null, undefined as raw function arguments tends to be unreadable. I always avoid it or comment them (e.g. ...getImmediate(/*identifier=*/undefined, ...))

[wilhuff]

Firestore is designed to support multiple instances per app. We happen to only expose a single (default) database right now but our intent is to expose many databases per cloud project.

I have no idea how that maps to multiple instances of Auth per instance.

On the whole though the thing that threw me is that undefined means DEFAULT_ENTRY_NAME = '[DEFAULT]'. I misinterpreted this to mean that it was the default app, not the default instance within the app. This could be avoided if there any comments (at all) about the meanings of these things in Provider.

[Feiyang1]

Yup. It's the identifier string for SDKs that allow multiple instances per app. You are right that it is insignificant for Firestore because Firestore doesn't support multiple instances per app.

It's intended for the following use case:

const app = firebase.initializeApp({
   ... 
  databaseUrl: "https://test.firebaseio.com",
 ...
});

// use the databaseUrl from the config
const database1 = app.database();

// user has 2 RTDB instances
// https://test-2.firebaseio.com is the identifier as well as the database url
// database2 is scoped to the same `app` and share the same auth state with database1
const database2 = app.database('https://test-2.firebaseio.com');

I like the idea of moving identifier to the options object because it hides identifier from SDKs that doesn't need it. I will make the change in #2316 and in other PRs.

[Feiyang1]

@wilhuff I added comments to Provider and updated getImmediate() signature as @mikelehen suggested.

[Feiyang1]

Yeah, I manually run tests for each SDK and made sure all tests pass before opening the PR. For travis to pass, we'd need to wait until all PRs are merged into fei-component-integration which will be merged into master eventually.

I tested auth and firestore integration manually in my test project by merging auth and firestore branches locally. I tried regular auth import, dynamic auth import which all works. Once everything merged into fei-component-integration, I will do an integration test again.

[Feiyang1]

@wilhuff Can I get the approval, so I can merge it into the integration branch for integration testing ?

[wilhuff]

LGTM