Typings Refactor #334

jshcrowthe · 2017-11-27T23:25:14Z

This PR refactors the single top level firebase package curated types into a series of 6 "types packages."

They are:

@firebase/app-types
@firebase/auth-types
@firebase/database-types
@firebase/firestore-types
@firebase/messaging-types
@firebase/storage-types

Each of these "types packages" defines the curated public interface and is used to generate our typings for the rest of the SDK.

Each component is encouraged to implement/extend these interfaces in their implementation directly (see the firestore package for a great example of this being done consistently throughout a codebase). Allowing the type system to validate that we are in fact exposing the interface we think we are.

Review Guide

All reviewers should take a look at the following commits as it pertains to their component (i.e. look at packages/app, packages/<COMPONENT>, packages/<COMPONENT>-types, and packages/firebase/<COMPONENT> in each of these commits:

d85410f - @firebase/app type refactor (@firebase/auth reviewer can skip this section)
9786607 - Adding the typings fields
51cff8d - Adding type package deps
086971e - Changes to the top level package

Broken down for each package owner are the package specific changes that were made:

Auth

Open to see relevant changes

26ba65f - Initial changes to auth structure
c7e9892 - Adding the @firebase/auth types

Database

Open to see relevant changes

f16c975 - Initial changes to database structure
0c93766 - Adding the @firebase/database types

Firestore

Open to see relevant changes

5b334fc - Initial changes to firestore structure
244ae14 - Adding the @firebase/firestore types

Messaging

Open to see relevant changes

376d80c - Initial changes to messaging structure
aa1f7b0 - Adding the @firebase/messaging types

Storage

Open to see relevant changes

fd26887 - Initial changes to storage structure
30e8354 - Adding the @firebase/storage types

There are a couple of cosmetic commits in there and a bugfix or two where an approach wasn't doing what I needed to do. Those commits don't actually change any code in what I have highlighted but move it to locations where everything works as expected.

Testing Flow

I manually validated these changes doing the following:

Run the following in your terminal (at the root of the package):
$(npm bin)/lerna exec --scope @firebase/* --scope firebase -- yarn link
Create a separate sandbox directory to test changes
Run yarn link <COMPONENT> for every component I intend to test

e.g.
```
$ yarn link @firebase/app
$ yarn link @firebase/messaging
```
Create a stub .ts file and import the components I linked.
Exercise the API as I'd expect it to work.

Testing

Additionally (and I will continue adding to this), I have added a test folder to each @fireabse/*-types package. In this I can exercise the API as I'd expect and ensure that the file properly compiles. This tests the validity of the typings with regards to our expected use cases. These files will never actually be run so I don't need to make them semantically correct, we simply are using them to test all expected use cases (this is how the typings package DefinitelyTyped tests the typings for each respective package.

mikelehen

I looked through the Firestore changes and overall I'm very happy with the way this turned out. I flagged a few minor-ish things (and one major thing that's probably responsible for firestore tests failing).

I didn't actually test / verify anything yet, but I'll do so tomorrow.

mikelehen · 2017-11-28T00:35:47Z

packages/app-types/index.d.ts

+};
+
+export class FirebaseApp {
+  constructor();


We don't support instantiating FirebaseApp directly, do we? So this should be private? (here and elsewhere... I see FirebaseAuth lists a public constructor too)

I pulled the public constructors from all the top level classes. Didn't realize they could be omitted.

mikelehen · 2017-11-28T00:37:33Z

packages/app-types/index.d.ts

+  /**
+   * Create (and intialize) a FirebaseApp.
+   *
+   * @param options Options to configure the services use in the App.


use => used

mikelehen · 2017-11-28T00:38:01Z

packages/app-types/index.d.ts

+   *
+   * @param options Options to configure the services use in the App.
+   * @param name The optional name of the app to initialize ('[DEFAULT]' if
+   *   none)


"if none" sounds weird to me. Perhaps "if omitted" (here and elsewhere)

mikelehen · 2017-11-28T00:41:38Z

packages/app-types/index.d.ts

+     * For testing FirebaseApp instances:
+     *  app() instanceof firebase.app.App
+     *
+     * DO NOT call this constuctor directly (use firebase.app() instead).


Firestore has hackery to prevent people from calling our constructors (see

firebase-js-sdk/packages/firestore/src/util/api.ts

Line 33 in 9f356b6

export function makeConstructorPrivate<T>(cls: T, optionalMessage?: string): T {

)

Perhaps we should do the same (and then you don't need the comment warning, since it just won't work)?

This would be a change to how the other APIs function currently, lets keep this option on the table though.

Not sure what you mean. What would be the change?

Calling the PublicFirestore constructor directly results in an Error being thrown, whereas the other components provide valid constructors to the firebase.. path.

It's a way to prevent people from instantiating the class themselves, but it is a change in the current behavior of the SDK. We do specify in our documentation that the constructors (presumably valid) exist at that path even though we say not to use it.

Either way, if this is a change we really want to pursue, I'd say let's do it in follow up rather than in this PR.

Okay, I'm fine with that. I just think it's ugly to have a big scary warning in the comments / docs when we could just disallow it... and since it's already documented as not supported, I wouldn't consider this a breaking change or anything. But I'm fine with punting on it for later. Firestore already does it, which is all I really care about. :-)

mikelehen · 2017-11-28T00:45:06Z

packages/app-types/index.d.ts

+  // es6-promises.
+  Promise: typeof Promise;
+
+  // The current SDK version ('${JSCORE_VERSION}').


I don't think JSCORE_VERSION will get replaced here, right?

Good call, I'll pull that.

mikelehen · 2017-11-28T01:04:22Z

packages/app-types/test/default.test.ts

+  const _name: string = app.name;
+  const _options: Object = app.options;
+  const _delete: Promise<any> = app.delete();
+});


I'm not exactly sure what is going on with this "test" file... Since we're not actually asserting anything, I think this is just compile-time verification (do we even execute this file? or just compile it?)... So it's basically a way for us to write against the .d.ts and sort of sanity-check that the type definitions are really what we think they are?

I find the value of this somewhat questionable (it doesn't provide much extra value than just reviewing the .d.ts directly), but if that is the intent, perhaps we should at least add a comment to the top of the file explaining the intent and perhaps change the "Assert that" wording, since there are no assertions here.

Note that if the intention is to verify that the runtime behavior matches our type definitions, I don't think it's very successful since e.g. there won't be any apps and so this forEach callback won't be run, and even if it was, if there was a mistake in the FirebaseApp typings (e.g. there isn't actually a .name property) this code likely wouldn't detect it since it's not asserting anything... (similarly the firebase.SDK_VERSION regex isn't asserted to pass, so it wouldn't test anything, etc.)

As I mentioned in the PR description, this is the "testing" that is done in the DefinitelyTyped repo (the only other repo I'm aware of that actively does curated types). I'll pull the wording, but the purpose of this file (and the others that need to be fleshed out) is to illustrate expected valid use cases of the API and make sure that they are considered type-safe.

Ah, sorry! FWIW, I think this makes more sense for DefinitivelyTyped than it does for us, since we (should) have real tests that run against the implementation and we should be able to compile those tests against the types (as Firestore does). So I would not be in favor of ever writing these sorts of compile-only tests against Firestore, since we already get that validation from our tests, so it'd just be an unnecessary maintenance burden. But I am fine with other packages relying on these tests if they want to...

mikelehen · 2017-11-28T01:13:20Z

packages/app-types/test/singleExport.test.ts

+ * limitations under the License.
+ */
+
+import { firebase } from '@firebase/app';


Is this file different than default.d.ts?

Yes, I am testing the default export vs. the named firebase export. Tested paths are identical but it's a different entrypoint.

mikelehen · 2017-11-28T01:28:17Z

packages/firestore/src/api/credentials.ts

@@ -155,7 +156,9 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
    this.userCounter = 0;

    // Will fire at least once where we set this.currentUser
-    this.app.INTERNAL.addAuthTokenListener(this.tokenListener);
+    (this.app as _FirebaseApp).INTERNAL.addAuthTokenListener(


Can you change app to be typed as _FirebaseApp and do the cast once at assignment time?

I opted to do it case-by-case as we don't need to access the INTERNAL namespace often regardless. Ultimately it will be going away so I'd rather leave the individual casts just as "REFACTOR THIS" flags 😄

Oh, interesting. When / why exactly will this be going away?

The INTERNAL namespace was used for code sharing. We are going to do something formal in this area to remove that need. It's not immediate, but soon.

mikelehen · 2017-11-28T01:41:43Z

packages/firestore/src/platform_node/grpc_connection.ts

@@ -131,7 +131,10 @@ export class GrpcConnection implements Connection {
      log.debug(LOG_TAG, 'Creating Firestore stub.');
      const credentials = createHeaders(this.databaseInfo, token);
      this.cachedStub = {
-        stub: new this.firestore.Firestore(this.databaseInfo.host, credentials),
+        stub: new this.firestore.FirebaseFirestore(


This is not correct, and tests should be failing!

mikelehen · 2017-11-28T01:57:37Z

packages/firestore-types/index.d.ts

+
+export type LogLevel = 'debug' | 'error' | 'silent';
+
+export function setLogLevel(logLevel: LogLevel): void;


It feels weird to be exporting a setLogLevel function (there is no "setLogLevel" function anywhere that you can directly import), but I think this whole firestore-types d.ts file is essentially an implementation detail (the "real" exports are in index.ts /index.node.ts) and so I guess this is fine, though looks a bit strange.

mikelehen · 2017-11-29T18:08:26Z

packages/firestore/test/integration/util/firebase_export.ts

@@ -23,4 +23,4 @@ import firebase from '@firebase/app';

 // TODO(b/66917182): This "as any" removes all of our type-checking in tests and
 // is therefore pretty bad. But I can't figure out how to avoid it right now.


Can you remove the comment too? :-)

Uhhh yes. (Derp moment... 😉 )

schmidt-sebastian

I added my comments for Database directly in the corresponding commits:

f16c975
0c93766

Thanks!

schmidt-sebastian · 2017-12-04T18:17:57Z

Good to go for the Database.

I do think we should go through and pick better parameter names for the callback types (result: error:) seems better than the existing (a: , b:), but this can be a separate effort.

bojeil-google · 2017-12-06T02:17:38Z