-
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
misc(viewer): fix types to reference LH.Result #7051
Conversation
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.
LGTM with two things
@@ -80,7 +80,7 @@ class ReportUIFeatures { | |||
/** | |||
* Adds export button, print, and other functionality to the report. The method | |||
* should be called whenever the report needs to be re-rendered. | |||
* @param {LH.ReportResult} report | |||
* @param {LH.Result} report | |||
*/ | |||
initFeatures(report) { |
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.
should also drop the Util.prepareReportResult
in report-ui-features-test.js
(that is a seriously anemic test file for the amount of jsdom setup it does :)
@@ -132,7 +132,7 @@ class LighthouseReportViewer { | |||
} | |||
|
|||
/** | |||
* @param {LH.ReportResult} json |
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.
Really all the callers to this are either doing it with any
or have casted any
, so this is kind of a lie.
json
should really have type unknown
and we could have _validateReportJson
verify that it's an LH.Result
(as it's superficially doing already), but that can be a project for another day.
Should we add a TODO or file an issue?
bd02934
to
10e9177
Compare
We're dealing with the real LHR here, not the ReportResult. Not sure how this happened but it's funny.
(EDIT: it was me :)