-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add explicit strict null checks in. #1763
Conversation
This commit also fixes the associated compile time issues that were encountered once these checks were enabled.
lighthouse-cli/bin.ts
Outdated
flags: {port: number, skipAutolaunch: boolean, selectChrome: boolean, output: any, | ||
outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean | ||
maxWaitForLoad: number}, | ||
config: Object): Promise<undefined> { | ||
config: Object | null): Promise<undefined> { | ||
|
||
let chromeLauncher: ChromeLauncher; |
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.
why is this declaration allowed by TS when we know it starts off as undefined :/
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.
if we know it starts out as undefined we should set it to config?: Object
It is optionally null since when we kick of
return runLighthouse(url, cliFlags, config)
config can be (is?) null
so we are assigning it to null.
/shrug
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.
Oh I'm referring to the bug in typescript allowing me to declare let chromeLauncher: ChromeLauncher;
when TS knows perfectly well that undefined
, it's initial value, is not a valid member of that type. I want it to force me to declare ChromeLauncher | undefined
@@ -31,17 +31,17 @@ const spawn = childProcess.spawn; | |||
const execSync = childProcess.execSync; | |||
const isWindows = process.platform === 'win32'; | |||
|
|||
class ChromeLauncher { | |||
export class ChromeLauncher { |
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.
just learning here, what difference does this make to TS as opposed to export {ChromeLauncher}
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.
none
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.
oh so just style?
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.
yeah just rolling it up a tiny bit
@@ -221,7 +221,7 @@ class ChromeLauncher { | |||
}); | |||
} | |||
|
|||
kill(): Promise<undefined> { | |||
kill(): Promise<{}> { |
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.
Interesting that these are not Promise<void>
I'm interested in these improvements. however i will point out this PR was precipitated by the fact that we couldn't catch the error in #1761 (we dont check for it appears that TS can't handle the promise flow here. a quick look and changing it to async/await does allow TS to spot the bug. i believe @samccone is trying that out. |
This gives us better type saftey around chrome launcher
lighthouse-cli/bin.ts
Outdated
if (typeof chromeLauncher !== 'undefined') { | ||
return chromeLauncher.kill().then(() => handleError(err), handleError); | ||
} | ||
let chromeLauncher: ChromeLauncher | undefined = undefined; |
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.
lol.
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.
it is the truth
return await chromeLauncher.kill(); | ||
} catch (err) { | ||
if (typeof chromeLauncher !== 'undefined') { | ||
await chromeLauncher!.kill(); |
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.
can you explain the !
? new to me.
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.
Non-null assertion operator
A new ! post-fix expression operator may be used to assert that its operand is non-null and non-undefined in contexts where the type checker is unable to conclude that fact. Specifically, the operation x! produces a value of the type of x with null and undefined excluded. Similar to type assertions of the forms x and x as T, the ! non-null assertion operator is simply removed in the emitted JavaScript code.
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.
and we need these due to a bug in typescript which I will file.
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.
EDIT: Just realized you might have been referring to a different bug than the one I'm duking out atm, but there it is anyway :)
No description provided.