-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
core(tsc): add type checking to remaining (dbw) gatherers #5005
Conversation
* @param {!Object} options | ||
* @return {!Promise<!Array<!Object>>} | ||
* @param {LH.Gatherer.PassContext} passContext | ||
* @return {!Promise<LH.Artifacts['JSLibraries']>} |
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.
no !
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.
💥
@@ -41,27 +42,34 @@ function installMediaListener() { | |||
} | |||
|
|||
/* istanbul ignore next */ |
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.
double check that this works being before the typedef 👍
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.
ok, it does work above or below the jsdoc block, but I moved it below just so it doesn't cause any second guessing some day :)
Unfortunately you still can't merge the comment blocks :(
@@ -21,8 +21,7 @@ | |||
"lighthouse-core/lib/emulation.js", | |||
"lighthouse-core/gather/computed/metrics/*.js", | |||
"lighthouse-core/gather/connections/**/*.js", | |||
"lighthouse-core/gather/gatherers/*.js", | |||
"lighthouse-core/gather/gatherers/seo/*.js", | |||
"lighthouse-core/gather/gatherers/**/*.js", |
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.
🎉
type checking now on for all gatherers*!
* with six of them left as
@ts-nocheck
These were a bit more convoluted than some of our more modern gatherers, but hopefully the changes aren't too much to review. No breaking changes; the only test changes needed were two switches from non-underscored properties to underscored ones.
@ts-nocheck
(domstats.js
andtags-blocking-first-paint.js
) but basic type checking and their artifacts were added.all-event-listeners.js
had the most surgery done as it was gathering way way more info thanno-mutation-events
needs. I cut down the collected info to just what was needed (or should obviously also be included).