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

Typings Refactor #334

Merged
merged 38 commits into from Dec 11, 2017

Conversation

@jshcrowthe
Contributor

jshcrowthe commented Nov 27, 2017

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:

  1. Run the following in your terminal (at the root of the package):
    $(npm bin)/lerna exec --scope @firebase/* --scope firebase -- yarn link

  2. Create a separate sandbox directory to test changes

  3. Run yarn link <COMPONENT> for every component I intend to test

    e.g.

    $ yarn link @firebase/app
    $ yarn link @firebase/messaging
    
  4. Create a stub .ts file and import the components I linked.

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

Show outdated Hide outdated packages/app-types/index.d.ts Outdated
Show outdated Hide outdated packages/app-types/index.d.ts Outdated
Show outdated Hide outdated packages/app-types/index.d.ts Outdated
* For testing FirebaseApp instances:
* app() instanceof firebase.app.App
*
* DO NOT call this constuctor directly (use firebase.app() instead).

This comment has been minimized.

@mikelehen

mikelehen Nov 28, 2017

Member

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

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)?

@mikelehen

mikelehen Nov 28, 2017

Member

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

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 comment has been minimized.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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

This comment has been minimized.

@mikelehen

mikelehen Nov 29, 2017

Member

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

@mikelehen

mikelehen Nov 29, 2017

Member

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

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 29, 2017

Contributor

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.

@jshcrowthe

jshcrowthe Nov 29, 2017

Contributor

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.

This comment has been minimized.

@mikelehen

mikelehen Nov 29, 2017

Member

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

mikelehen Nov 29, 2017

Member

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. :-)

Show outdated Hide outdated packages/app-types/index.d.ts Outdated
const _name: string = app.name;
const _options: Object = app.options;
const _delete: Promise<any> = app.delete();
});

This comment has been minimized.

@mikelehen

mikelehen Nov 28, 2017

Member

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

@mikelehen

mikelehen Nov 28, 2017

Member

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

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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.

This comment has been minimized.

@mikelehen

mikelehen Nov 29, 2017

Member

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

mikelehen Nov 29, 2017

Member

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

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

This comment has been minimized.

@mikelehen

mikelehen Nov 28, 2017

Member

Is this file different than default.d.ts?

@mikelehen

mikelehen Nov 28, 2017

Member

Is this file different than default.d.ts?

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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

@@ -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(

This comment has been minimized.

@mikelehen

mikelehen Nov 28, 2017

Member

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

@mikelehen

mikelehen Nov 28, 2017

Member

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

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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 😄

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

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 😄

This comment has been minimized.

@mikelehen

mikelehen Nov 29, 2017

Member

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

@mikelehen

mikelehen Nov 29, 2017

Member

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

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 29, 2017

Contributor

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.

@jshcrowthe

jshcrowthe Nov 29, 2017

Contributor

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.

Show outdated Hide outdated packages/firestore/src/platform_node/grpc_connection.ts Outdated
export type LogLevel = 'debug' | 'error' | 'silent';
export function setLogLevel(logLevel: LogLevel): void;

This comment has been minimized.

@mikelehen

mikelehen Nov 28, 2017

Member

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

mikelehen Nov 28, 2017

Member

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.

This comment has been minimized.

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

👍

@jshcrowthe

jshcrowthe Nov 28, 2017

Contributor

👍

Josh Crowther added some commits Nov 29, 2017

Josh Crowther
Josh Crowther
Josh Crowther
@schmidt-sebastian

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

f16c975
0c93766

Thanks!

@jshcrowthe jshcrowthe removed the needs-triage label Dec 4, 2017

@schmidt-sebastian

This comment has been minimized.

Show comment
Hide comment
@schmidt-sebastian

schmidt-sebastian Dec 4, 2017

Member

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.

Member

schmidt-sebastian commented Dec 4, 2017

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.

Josh Crowther added some commits Dec 4, 2017

Josh Crowther
Josh Crowther
Josh Crowther
applyActionCode(code: string): Promise<any>;
checkActionCode(code: string): Promise<any>;
confirmPasswordReset(code: string, newPassword: string): Promise<any>;
createUserWithEmailAndPassword(email: string, password: string): Promise<any>;

This comment has been minimized.

@bojeil-google

bojeil-google Dec 6, 2017

Contributor

Can you add the following 4 methods that are being released this week:
createUserAndRetrieveDataWithEmailAndPassword(email: string, password: string): Promise<any>;

@bojeil-google

bojeil-google Dec 6, 2017

Contributor

Can you add the following 4 methods that are being released this week:
createUserAndRetrieveDataWithEmailAndPassword(email: string, password: string): Promise<any>;

This comment has been minimized.

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

Done

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

Done

): Promise<any>;
setPersistence(persistence: Persistence): Promise<any>;
signInAndRetrieveDataWithCredential(credential: AuthCredential): Promise<any>;
signInAnonymously(): Promise<any>;

This comment has been minimized.

@bojeil-google

bojeil-google Dec 6, 2017

Contributor

Add:
signInAnonymouslyAndRetrieveData(): Promise<any>;

@bojeil-google

bojeil-google Dec 6, 2017

Contributor

Add:
signInAnonymouslyAndRetrieveData(): Promise<any>;

This comment has been minimized.

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

Done

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

Done

Show outdated Hide outdated packages/auth-types/index.d.ts Outdated
Show outdated Hide outdated packages/auth-types/index.d.ts Outdated
}
declare module '@firebase/app-types' {
interface FirebaseNamespace {

This comment has been minimized.

@bojeil-google

bojeil-google Dec 6, 2017

Contributor

This comment has been minimized.

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

I have updated the typings and added the classes mentioned, PTAL.

@jshcrowthe

jshcrowthe Dec 7, 2017

Contributor

I have updated the typings and added the classes mentioned, PTAL.

@sphippen

Storage looks good.

Josh Crowther added some commits Dec 7, 2017

Josh Crowther
Josh Crowther

@jshcrowthe jshcrowthe merged commit a6b6689 into master Dec 11, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jshcrowthe jshcrowthe deleted the typings branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment