Enable noImplicitAny Typescript compiler flag #1862
Conversation
hsubox76
requested review from
davideast,
Feiyang1,
hiranya911,
jamesdaniels,
jsdt,
mikelehen,
ryanpbrewster,
schmidt-sebastian,
scottcrossen and
yuchenshi
as code owners
| import { | ||
| FirebaseServiceFactory, | ||
| _FirebaseNamespace | ||
| } from '@firebase/app-types/private'; |
There was a problem hiding this comment.
Could this just be FirebaseNamespace? It's not immediately obvious to me why it needs an underscore, but I am willing to learn :)
There was a problem hiding this comment.
Hm, I guess we have:
import { FirebaseNamespace } from '@firebase/app-types';
import { _FirebaseNamespace } from '@firebase/app-types/private';
There was a problem hiding this comment.
Sure, the public FirebaseNamespace type doesn't have an INTERNAL property which causes a TS error on line 54.
| @@ -163,7 +163,7 @@ export function list( | |||
| pageToken?: string | null, | |||
| maxResults?: number | null | |||
| ): RequestInfo<ListResult> { | |||
| let urlParams = {}; | |||
| let urlParams: { [key: string]: any } = {}; | |||
There was a problem hiding this comment.
We have another PR pending that removes any from Storage. Can you use unknown here?
There was a problem hiding this comment.
I actually just saw that there was a UrlParams type so I used that. The only change is I have to explicitly cast maxResults to string if that's okay.
There was a problem hiding this comment.
Oops I guess it wasn't okay. I changed UrlParams to accept string | number values, that seems like a better idea.
packages/app/index.node.ts
Outdated
| import { XMLHttpRequest } from 'xmlhttprequest'; | ||
| // Node specific packages. | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const Storage = require('dom-storage'); |
There was a problem hiding this comment.
What's the motivation behind the change?
There was a problem hiding this comment.
dom-storage and xmlhttprequest don't have types so the type of Storage and XMLHttpRequest is implicitly any. Maybe it would be simpler to just @ts-ignore and explain it is a node package.
| @@ -80,6 +80,7 @@ export function createFirebaseNamespaceCore( | |||
| // | |||
| // import * as firebase from 'firebase'; | |||
| // which becomes: var firebase = require('firebase'); | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
As discussed, please cast to any and use eslint suppression instead of @ts-ignore.
packages/functions/src/serializer.ts
Outdated
| function mapValues(o: object, f: (any) => any): object { | ||
| let result = {}; | ||
| function mapValues(o: { [key: string]: any }, f: (arg0: any) => any): object { | ||
| let result: { [key: string]: any } = {}; |
There was a problem hiding this comment.
should use unknown instead. No action is needed . Explicit any is being tackled in eslint PRs.
| try { | ||
| sizeString = xhr.getResponseHeader('X-Goog-Upload-Size-Received'); | ||
| if (sizeString === null) { |
There was a problem hiding this comment.
This check is not needed. parseInt will produce NaN which will be caught by isNaN check.
| @@ -212,7 +212,8 @@ export function loadFirestoreRules( | |||
| project: `projects/${options.projectId}`, | |||
| rules: { files: [{ content: options.rules }] } | |||
| }, | |||
| (err, resp) => { | |||
| // @ts-ignore Defined in protobuf. | |||
There was a problem hiding this comment.
resolve() won't accept unknown
packages/util/src/jwt.ts
Outdated
| @@ -18,26 +18,34 @@ | |||
| import { base64Decode } from './crypt'; | |||
| import { jsonEval } from './json'; | |||
|
|
|||
| interface Claims { | |||
| [key: string]: object | number | string | boolean; | |||
There was a problem hiding this comment.
just use {} as type ? {} also includes symbol, but it should be fine.
packages/util/src/jwt.ts
Outdated
| if (typeof claims === 'object' && claims.hasOwnProperty('iat')) { | ||
| return claims['iat']; | ||
| return claims['iat'] as number | null; |
packages/util/src/obj.ts
Outdated
| @@ -18,12 +18,20 @@ | |||
| /* eslint-disable */ | |||
| // will enable after https://github.com/firebase/firebase-js-sdk/pull/1811 is merged | |||
|
|
|||
| interface UtilObject<V> { | |||
| [key: string]: V; | |||
| [index: number]: V; | |||
There was a problem hiding this comment.
No need to define a number index. All number indexing are string indexing under the hood.
There was a problem hiding this comment.
Weird, I was previously getting a lot of errors in places that used this functions with number-indexed objects but I guess those errors got fixed on the consuming end. Done.
* Fix packages/firestore/ usages of @ts-ignore. I left the equality_matcher.ts ones for the code we copy/pasted in, but fixed the rest.
Enabled noImplicitAny Typescript compiler flag on all packages with an exception for database, which would require extensive updates out of scope for this PR.