-
Notifications
You must be signed in to change notification settings - Fork 396
Upgrading tslint to 5.9.0 #194
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
Conversation
@@ -320,9 +319,9 @@ export class FirebaseAuthRequestHandler { | |||
* Looks up a user by uid. | |||
* | |||
* @param {string} uid The uid of the user to lookup. | |||
* @return {Promise<Object>} A promise that resolves with the user information. | |||
* @return {Promise<object>} A promise that resolves with the user information. |
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.
Does the linter complain about javadoc types too?
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.
Not really, but I thought it makes sense to keep the jsdoc types in sync with the implementation. WDYT?
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.
The downside is that it will not align with our externs format. Could this be an issue?
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.
I think that's fine since the externs are curated by hand anyway. Going forward we can adhere to the distinction:
- Only Typescript in source (everything on Github)
- Only JS in API docs (externs)
src/auth/auth-api-request.ts
Outdated
error = response; | ||
} | ||
|
||
const error = (typeof response === 'object' && 'statusCode' in response) |
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.
Better for readability to have the ? on the previous line:
const error = (typeof response === 'object' && 'statusCode' in response) ?
response.error : response;
Here and elsewhere
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.
Done
src/auth/credential.ts
Outdated
} | ||
constructor(serviceAccountPathOrObject: string | object) { | ||
this.certificate_ = (typeof serviceAccountPathOrObject === 'string') | ||
? Certificate.fromPath(serviceAccountPathOrObject) : new Certificate(serviceAccountPathOrObject); |
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.
Better for readability to have the ? on the previous line
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.
Done
src/auth/credential.ts
Outdated
} | ||
constructor(refreshTokenPathOrObject: string | object) { | ||
this.refreshToken_ = (typeof refreshTokenPathOrObject === 'string') | ||
? RefreshToken.fromPath(refreshTokenPathOrObject) : new RefreshToken(refreshTokenPathOrObject); |
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.
Better for readability to have the ? on the previous line
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.
Done
} | ||
|
||
const error = (typeof response === 'object' && 'error' in response) | ||
? response.error : response; |
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.
Better for readability to have the ? on the previous line
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.
Done
} | ||
const template: string = ERROR_CODES[response.statusCode]; | ||
const message: string = template | ||
? `Instance ID "${apiSettings.getEndpoint()}": ${template}` : JSON.stringify(error); |
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.
Better for readability to have the ? on the previous line
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.
Done
Upgrading to a more recent version of tslint, which performs a wide range of checks on source. I've updated the source to be compliant.