-
Notifications
You must be signed in to change notification settings - Fork 927
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
Typings Refactor #334
Changes from 28 commits
cd609c1
54eed48
e357596
df576c0
a49c127
003b753
f63d6ef
9be39d6
c307b9c
f1d9a27
e9003f9
4cfb6d8
f939177
a4f9af4
bc323e3
f89dbc9
ac4a523
fdad69c
09571e0
e2e6ffb
282beb2
2e8141e
0dd8060
d1f0fcd
e7fcf96
ffdbd1e
08a5451
c8d8267
55c362e
8693c96
47d50f0
d6d0364
424def2
d57cbd8
2e8d551
8eab528
d8e6fa8
b0813c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,85 @@ | ||||
/** | ||||
* 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). | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firestore has hackery to prevent people from calling our constructors (see
Perhaps we should do the same (and then you don't need the comment warning, since it just won't work)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean. What would be the change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||
} |
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" | ||
} | ||
} |
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; | ||
}; | ||
} |
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(); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); |
There was a problem hiding this comment.
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:
With this refactor, the "as any" is no longer required.
(and then you can close http://b/66917182)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha!