Skip to content
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: add type checking to audit and gatherer base classes #4762

Merged
merged 6 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 51 additions & 17 deletions lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
// @ts-nocheck
'use strict';

const statistics = require('../lib/statistics');
Expand All @@ -19,14 +18,14 @@ const clampTo2Decimals = val => Math.round(val * 100) / 100;

class Audit {
/**
* @return {!string}
* @return {string}
*/
static get DEFAULT_PASS() {
return DEFAULT_PASS;
}

/**
* @return {{NUMERIC: string, BINARY: string}}
* @return {Audit.ScoringModes}
*/
static get SCORING_MODES() {
return {
Expand All @@ -36,14 +35,14 @@ class Audit {
}

/**
* @throws {Error}
* @return {Audit.Meta}
*/
static get meta() {
throw new Error('Audit meta information must be overridden.');
}

/**
* Computes a clamped score between 0 and 100 based on the measured value. Score is determined by
* Computes a clamped score between 0 and 1 based on the measured value. Score is determined by
* considering a log-normal distribution governed by the two control points, point of diminishing
* returns and the median value, and returning the percentage of sites that have higher value.
*
Expand All @@ -65,9 +64,9 @@ class Audit {
}

/**
* @param {!Audit} audit
* @param {typeof Audit} audit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this mean btw?

Copy link
Member Author

@brendankenny brendankenny Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit is the name of the class in js-land, but in type-land it's the name of the type of Audit instances.

To refer to the type of the class Audit itself (since we use all static methods) you have to use typeof, which is kind of unfortunately overloaded in TS but gets the job done and isn't too obscure...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh! thanks for explaining.
wish it was instanceof instead.

Copy link
Member Author

@brendankenny brendankenny Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately you almost always want the normal thing. It's just here we're not just using all static methods, we want the static methods on subclasses of Audit, so we have to pass the subclass in to Audit's static methods... 🤪

* @param {string} debugString
* @return {!AuditFullResult}
* @return {LH.AuditFullResult}
*/
static generateErrorAuditResult(audit, debugString) {
return Audit.generateAuditResult(audit, {
Expand All @@ -78,10 +77,10 @@ class Audit {
}

/**
* @param {!Audit.Headings} headings
* @param {!Array<!Object<string, string>>} results
* @param {!DetailsRenderer.DetailsSummary} summary
* @return {!DetailsRenderer.DetailsJSON}
* @param {Audit.Headings} headings
* @param {Array<Object<string, string>>} results
* @param {Audit.DetailsRenderer.DetailsSummary} summary
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stuck these on Audit to keep them local since they're going to be pulled in by the larger LHR/lite effort. summary is defined as void for now to encourage adding types :)

* @return {Audit.DetailsRenderer.DetailsJSON}
*/
static makeTableDetails(headings, results, summary) {
if (results.length === 0) {
Expand All @@ -102,9 +101,9 @@ class Audit {
}

/**
* @param {!Audit} audit
* @param {!AuditResult} result
* @return {{score: number, scoreDisplayMode: string}}
* @param {typeof Audit} audit
* @param {LH.AuditResult} result
* @return {{score: number, scoreDisplayMode: Audit.ScoringModeValues}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit.ScoringModeValues vs @typedef {Object} Audit.ScoringModes below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScoringModeValues is one of the string values of the Audit.ScoringModes object (so just 'numeric' | 'binary'). We could probably fix this up better, maybe if we pull most of the audit stuff into here and/or an audit.d.ts file

*/
static _normalizeAuditScore(audit, result) {
// Cast true/false to 1/0
Expand All @@ -125,9 +124,9 @@ class Audit {
}

/**
* @param {!Audit} audit
* @param {!AuditResult} result
* @return {!AuditFullResult}
* @param {typeof Audit} audit
* @param {LH.AuditResult} result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that's been bugging me lately.. In our external LHR docs we want to basically call LH.AuditFullResult the "Audit Result". So one alternative here is...

  • LH.AuditResult => something.. (like LH.AuditReturnValue) and
  • LH.AuditFullResult => LH.AuditResult

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the problem used to be we needed a nice name in each audit and we didn't care so much what came out the other side. I don't mind the switch but we should come up with a good alternative for the first one (and maybe punt on this because there are a lot of AuditResults that would need to change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with punting for today.

* @return {LH.AuditFullResult}
*/
static generateAuditResult(audit, result) {
if (typeof result.rawValue === 'undefined') {
Expand Down Expand Up @@ -175,6 +174,28 @@ class Audit {

module.exports = Audit;

/**
* @typedef {Object} Audit.ScoringModes
* @property {'numeric'} NUMERIC
* @property {'binary'} BINARY
*/

/**
* @typedef {Audit.ScoringModes[keyof Audit.ScoringModes]} Audit.ScoringModeValues
*/

/**
* @typedef {Object} Audit.Meta
* @property {string} name
* @property {string} description
* @property {string} helpText
* @property {Array<string>} requiredArtifacts
* @property {string=} failureDescription
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you feel about doing foo|undefined instead of foo=? i can almost promise i'll forget what foo= means in another 3 weeks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the explicitness, especially because it's only allowed in some circumstances, but on the other hand it can get pretty unwieldy and there's no jsdoc equivalent to the nice ?: on the ts side

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k can we do foo|undefined?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh but there is! http://usejsdoc.org/tags-property.html :)

/**
 * @property {Array<string>} requiredArtifacts
 * @property {string} [failureDescription]
 */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that...does not seem better than = :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems slightly better to me, but not as good as ? or |undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you guys are crazy, optional is part of the type not the name!

luckily we can all agree on good old boring dependable |undefined

Copy link
Collaborator

@patrickhulce patrickhulce Mar 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you guys are crazy, optional is part of the type not the name!

but the semantics are different!

|undefined implies the key is always there but just potentially as undefined, the [prop] syntax more correctly suggests that the property name is optional rather than the type of the value is sometimes undefined :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript treats {type=} and [propName] the same, but the distinction from |undefined is important...and I was wrong before about |undefined. tsc isn't going to let us use it anyways with --strict on.

The difference is erased when you get something that is this type, but not when declaring that something is one of these types...e.g. with @property {boolean|undefined} informative you better have a property informative that's one of the two. A missing informative won't suffice, so the property is no longer optional.

I like this reasoning of [prop], it offers a nice way of expressing the defined/not-defined vs value/undefined mixup of JS, so let's go with that.

* @property {boolean=} informative
* @property {boolean=} manual
* @property {Audit.ScoringModeValues=} scoreDisplayMode
*/

/**
* @typedef {Object} Audit.Heading
* @property {string} key
Expand All @@ -193,3 +214,16 @@ module.exports = Audit;
* @property {boolean} passes
* @property {string=} debugString
*/

// TODO: placeholder typedefs until Details are typed
/**
* @typedef {void} Audit.DetailsRenderer.DetailsSummary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could go with this for now:

 wastedMs?: number
 wastedKb?: number

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

*/

/**
* @typedef {object} Audit.DetailsRenderer.DetailsJSON
* @property {'table'} type
* @property {Array<Audit.Heading>} headings
* @property {Array<Object<string, string>>} items
* @property {void} summary
*/
28 changes: 21 additions & 7 deletions lighthouse-core/gather/gatherers/gatherer.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,42 @@ class Gatherer {

/**
* Called before navigation to target url.
* @param {!Object} options
* @param {Gatherer.PassContext} passContext
* @return {*|!Promise<*>}
*/
beforePass(options) { }
beforePass(passContext) { }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃


/**
* Called after target page is loaded. If a trace is enabled for this pass,
* the trace is still being recorded.
* @param {!Object} options
* @param {Gatherer.PassContext} passContext
* @return {*|!Promise<*>}
*/
pass(options) { }
pass(passContext) { }

/**
* Called after target page is loaded, all gatherer `pass` methods have been
* executed, and — if generated in this pass — the trace is ended. The trace
* and record of network activity are provided in `loadData`.
* @param {!Object} options
* @param {{networkRecords: !Array, trace: {traceEvents: !Array}}} loadData
* @param {Gatherer.PassContext} passContext
* @param {Gatherer.LoadData} loadData
* @return {*|!Promise<*>}
*/
afterPass(options, loadData) { }
afterPass(passContext, loadData) { }

/* eslint-enable no-unused-vars */
}

/**
* @typedef {Object} Gatherer.PassContext
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

* @property {object} options
*/

/**
* @typedef {Object} Gatherer.LoadData
* @property {Array<LH.NetworkRequest>} networkRecords
* @property {Array<void>} devtoolsLog
* @property {{traceEvents: Array<LH.TraceEvent>}} trace
*/

module.exports = Gatherer;
2 changes: 2 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
},
"include": [
"lighthouse-cli/**/*.js",
"lighthouse-core/audits/audit.js",
"lighthouse-core/lib/dependency-graph/**/*.js",
"lighthouse-core/gather/connections/**/*.js",
"lighthouse-core/gather/gatherers/gatherer.js",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add audit.js too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, better to be explicit

"./typings/externs.d.ts"
],
"exclude": [
Expand Down
Loading