Skip to content

Typings Refactor #334

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 38 commits into from
Dec 11, 2017
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
cd609c1
Add @firebase/app-types
jshcrowthe Nov 21, 2017
54eed48
Add @firebase/auth-types stub
jshcrowthe Nov 21, 2017
e357596
Add @firebase/database-types stub
jshcrowthe Nov 21, 2017
df576c0
Add @firebase/firestore-types stub
jshcrowthe Nov 21, 2017
a49c127
Add @firebase/messaging-types stub
jshcrowthe Nov 21, 2017
003b753
Add @firebase/storage-types stub
jshcrowthe Nov 21, 2017
f63d6ef
Add "typings" field to components package.json
jshcrowthe Nov 21, 2017
9be39d6
Add typings dependencies to each component
jshcrowthe Nov 21, 2017
c307b9c
Add @firebase/messaging-types
jshcrowthe Nov 21, 2017
f1d9a27
Add @firebase/storage-types
jshcrowthe Nov 21, 2017
e9003f9
Add @firebase/auth-types
jshcrowthe Nov 22, 2017
4cfb6d8
Refactor the Persistence object on FirebaseAuth class
jshcrowthe Nov 22, 2017
f939177
Add @firebase/firestore types
jshcrowthe Nov 27, 2017
a4f9af4
Fix compilation type module require issues
jshcrowthe Nov 27, 2017
bc323e3
Add @firebase/database types
jshcrowthe Nov 27, 2017
f89dbc9
Fix issues in firestore/storage types
jshcrowthe Nov 27, 2017
ac4a523
[AUTOMATED]: Prettier Code Styling
jshcrowthe Nov 27, 2017
fdad69c
[AUTOMATED]: License Headers
jshcrowthe Nov 27, 2017
09571e0
Refactor firebase package types
jshcrowthe Nov 27, 2017
e2e6ffb
[AUTOMATED]: Prettier Code Styling
jshcrowthe Nov 27, 2017
282beb2
[AUTOMATED]: License Headers
jshcrowthe Nov 27, 2017
2e8141e
Review feedback from @mikelehen
jshcrowthe Nov 28, 2017
0dd8060
[AUTOMATED]: Prettier Code Styling
jshcrowthe Nov 28, 2017
d1f0fcd
Reintroduce private constructors
jshcrowthe Nov 29, 2017
e7fcf96
Remove "as any" cast in firebase_export.ts
jshcrowthe Nov 29, 2017
ffdbd1e
[AUTOMATED]: Prettier Code Styling
jshcrowthe Nov 29, 2017
08a5451
Merge branch 'master' into typings
jshcrowthe Nov 29, 2017
c8d8267
Removing unneeded comment
jshcrowthe Nov 29, 2017
55c362e
Address feedback from @schmidt-sebastian
jshcrowthe Dec 4, 2017
8693c96
Merge branch 'master' into typings
jshcrowthe Dec 4, 2017
47d50f0
Merge branch 'master' into typings
jshcrowthe Dec 5, 2017
d6d0364
Merge branch 'master' into typings
jshcrowthe Dec 5, 2017
424def2
Merge branch 'master' into typings
jshcrowthe Dec 7, 2017
d57cbd8
Fix disparities with prod externs
jshcrowthe Dec 7, 2017
2e8d551
Adding new @firebase/auth methods
jshcrowthe Dec 7, 2017
8eab528
[AUTOMATED]: Prettier Code Styling
jshcrowthe Dec 7, 2017
d8e6fa8
Merge branch 'master' into typings
jshcrowthe Dec 7, 2017
b0813c5
Merge branch 'master' into typings
jshcrowthe Dec 8, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions packages/app-types/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this file, can you remove this hack from packages/firestore/test/integration/util/firebase_export.ts:

// 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.
export default firebase as any;

With this refactor, the "as any" is no longer required.

(and then you can close http://b/66917182)

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!

* Copyright 2017 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export type FirebaseOptions = {
apiKey?: string;
authDomain?: string;
databaseURL?: string;
projectId?: string;
storageBucket?: string;
messagingSenderId?: string;
[name: string]: any;
};

export class FirebaseApp {
/**
* The (read-only) name (identifier) for this App. '[DEFAULT]' is the default
* App.
*/
name: string;

/**
* The (read-only) configuration options from the app initialization.
*/
options: FirebaseOptions;

/**
* Make the given App unusable and free resources.
*/
delete(): Promise<void>;
}

export interface FirebaseNamespace {
/**
* Create (and intialize) a FirebaseApp.
*
* @param options Options to configure the services used in the App.
* @param name The optional name of the app to initialize ('[DEFAULT]' if
* omitted)
*/
initializeApp(options: FirebaseOptions, name?: string): FirebaseApp;

app: {
/**
* Retrieve an instance of a FirebaseApp.
*
* Usage: firebase.app()
*
* @param name The optional name of the app to return ('[DEFAULT]' if omitted)
*/
(name?: string): FirebaseApp;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

*/
App: typeof FirebaseApp;
};

/**
* A (read-only) array of all the initialized Apps.
*/
apps: FirebaseApp[];

// Inherit the type information of our exported Promise implementation from
// es6-promises.
Promise: typeof Promise;

// The current SDK version.
SDK_VERSION: string;
}
22 changes: 22 additions & 0 deletions packages/app-types/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"name": "@firebase/app-types",
"version": "0.1.0",
"description": "@firebase/app Types",
"license": "Apache-2.0",
"scripts": {
"test": "tsc"
},
"files": [
"index.d.ts"
],
"devDependencies": {
"typescript": "^2.4.2"
},
"repository": {
"type": "git",
"url": "https://github.com/firebase/firebase-js-sdk/tree/master/packages/app-types"
},
"bugs": {
"url": "https://github.com/firebase/firebase-js-sdk/issues"
}
}
159 changes: 159 additions & 0 deletions packages/app-types/private.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/**
* Copyright 2017 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* THIS FILE IS FOR INTERNAL USAGE ONLY, IF YOU ARE NOT DEVELOPING THE FIREBASE
* SDKs, PLEASE DO NOT REFERENCE THIS FILE AS IT MAY CHANGE WITHOUT WARNING
*/

import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
import { Observer, Subscribe } from '@firebase/util';
import { FirebaseError } from '@firebase/util';

export interface FirebaseServiceInternals {
/**
* Delete the service and free it's resources - called from
* app.delete().
*/
delete(): Promise<void>;
}

// Services are exposed through instances - each of which is associated with a
// FirebaseApp.
export interface FirebaseService {
app: FirebaseApp;
INTERNAL?: FirebaseServiceInternals;
}

export type AppHook = (event: string, app: FirebaseApp) => void;

/**
* Firebase Services create instances given a Firebase App instance and can
* optionally add properties and methods to each FirebaseApp via the extendApp()
* function.
*/
export interface FirebaseServiceFactory {
(
app: FirebaseApp,
extendApp?: (props: { [prop: string]: any }) => void,
instanceString?: string
): FirebaseService;
}

/**
* All ServiceNamespaces extend from FirebaseServiceNamespace
*/
export interface FirebaseServiceNamespace<T extends FirebaseService> {
(app?: FirebaseApp): T;
}

export interface FirebaseErrorFactory<T> {
create(code: T, data?: { [prop: string]: any }): FirebaseError;
}

export interface FirebaseErrorFactoryClass {
new (
service: string,
serviceName: string,
errors: { [code: string]: string }
): FirebaseErrorFactory<any>;
}

export interface FirebaseAuthTokenData {
accessToken: string;
}

export interface FirebaseAppInternals {
getToken(refreshToken?: boolean): Promise<FirebaseAuthTokenData | null>;
getUid(): string | null;
addAuthTokenListener(fn: (token: string | null) => void): void;
removeAuthTokenListener(fn: (token: string | null) => void): void;
}

export interface _FirebaseApp extends FirebaseApp {
INTERNAL: FirebaseAppInternals;
}
export interface _FirebaseNamespace extends FirebaseNamespace {
INTERNAL: {
/**
* Internal API to register a Firebase Service into the firebase namespace.
*
* Each service will create a child namespace (firebase.<name>) which acts as
* both a namespace for service specific properties, and also as a service
* accessor function (firebase.<name>() or firebase.<name>(app)).
*
* @param name The Firebase Service being registered.
* @param createService Factory function to create a service instance.
* @param serviceProperties Properties to copy to the service's namespace.
* @param appHook All appHooks called before intializeApp returns to caller.
* @param allowMultipleInstances Whether the registered service supports
* multiple instances per app. If not specified, the default is false.
*/
registerService(
name: string,
createService: FirebaseServiceFactory,
serviceProperties?: { [prop: string]: any },
appHook?: AppHook,
allowMultipleInstances?: boolean
): FirebaseServiceNamespace<FirebaseService>;

/**
* Just used for testing to start from a fresh namespace.
*/
createFirebaseNamespace(): FirebaseNamespace;

/**
* Internal API to install properties on the top-level firebase namespace.
* @prop props The top level properties of this object are copied to the
* namespace.
*/
extendNamespace(props: { [prop: string]: any }): void;

/**
* Create a Subscribe function. A proxy Observer is created so that
* events can be sent to single Observer to be fanned out automatically.
*/
createSubscribe<T>(
executor: (observer: Observer<T, Error>) => void,
onNoObservers?: (observer: Observer<T, Error>) => void
): Subscribe<T>;

/**
* Utility exposed for internal testing.
*/
deepExtend(target: any, source: any): any;

/**
* Internal API to remove an app from the list of registered apps.
*/
removeApp(name: string): void;

/**
* Service factories for each registered service.
*/
factories: { [name: string]: FirebaseServiceFactory };

/*
* Convert service name to factory name to use.
*/
useAsService(app: FirebaseApp, serviceName: string): string | null;

/**
* Use to construct all thrown FirebaseError's.
*/
ErrorFactory: FirebaseErrorFactoryClass;
};
}
99 changes: 99 additions & 0 deletions packages/app-types/test/default.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* Copyright 2017 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import firebase from '@firebase/app';

/**
* Assert `firebase.apps` is an array of Firebase apps
*
* i.e. We should be able to access the public members
* of the App from inside of an individual app.
*/
firebase.apps.forEach(app => {
const _name: string = app.name;
const _options: Object = app.options;
const _delete: Promise<any> = app.delete();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
* Assert the SDK_VERSION is a string by passing it to
* regex.test
*/
const regex = /\d\.\d\.\d/;
regex.test(firebase.SDK_VERSION);

/**
* Assert we can init w/ a partially empty config
* object
*/
firebase.initializeApp({
apiKey: '1234567890'
});

/**
* Assert we can init w/ full config object
*/
firebase.initializeApp({
apiKey: '1234567890',
authDomain: '1234567890',
databaseURL: '1234567890',
projectId: '1234567890',
storageBucket: '1234567890',
messagingSenderId: '1234567890'
});

/**
* Assert we can pass an optional name
*/
firebase.initializeApp(
{
apiKey: '1234567890',
authDomain: '1234567890',
databaseURL: '1234567890',
projectId: '1234567890',
storageBucket: '1234567890',
messagingSenderId: '1234567890'
},
'Dummy Name'
);

/**
* Assert we get an instance of a FirebaseApp from `firebase.app()`
*/
const app = firebase.app();

/**
* Assert the `name` and `options` properties exist
*/
const _name: string = app.name;
const _options: Object = app.options;

/**
* Assert the `delete` method exists and returns a `Promise`
*/
const _delete: Promise<void> = app.delete();

/**
* Assert the `Promise` ctor mounted at `firebase.Promise` can
* be used to create new Promises.
*/

const promise: Promise<any> = new firebase.Promise((resolve, reject) => {
resolve({});
reject({});
})
.then()
.catch();
Loading