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 all 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
72 changes: 54 additions & 18 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]
* @property {boolean} [informative]
* @property {boolean} [manual]
* @property {Audit.ScoringModeValues} [scoreDisplayMode]
*/

/**
* @typedef {Object} Audit.Heading
* @property {string} key
Expand All @@ -191,5 +212,20 @@ module.exports = Audit;
* @property {number} results
* @property {Audit.Headings} headings
* @property {boolean} passes
* @property {string=} debugString
* @property {string} [debugString]
*/

// TODO: placeholder typedefs until Details are typed
/**
* @typedef {void} Audit.DetailsRenderer.DetailsSummary
* @property {number} [wastedMs]
* @property {number} [wastedKb]
*/

/**
* @typedef {object} Audit.DetailsRenderer.DetailsJSON
* @property {'table'} type
* @property {Array<Audit.Heading>} headings
* @property {Array<Object<string, string>>} items
* @property {Audit.DetailsRenderer.DetailsSummary} 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",
"./typings/externs.d.ts"
],
"exclude": [
Expand Down
20 changes: 11 additions & 9 deletions typings/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* 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.
*/

export as namespace LH;
declare namespace LH {
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.

turns out export as namespace was really only intended for UMD modules. This is the "proper" way to declare a global type namespace (in a d.ts file). The old way will break in the next release of TS so might as well do it correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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


export interface Flags {
_: string[];
Expand Down Expand Up @@ -34,29 +34,30 @@ export interface Flags {
export interface Config {}

export interface AuditResult {
rawValue: boolean | number;
rawValue: boolean | number | null;
Copy link
Member

Choose a reason for hiding this comment

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

we need null?

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, audit error results have rawValue of null (generateErrorAuditResult). Scoring seems to not really care, but we have unit tests asserting it so I just left it as is

Copy link
Member

Choose a reason for hiding this comment

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

mmmmmmmmmmmkkkkkkkkkkkkkkkkkkkkayyyyyyyyyyy.

displayValue?: string;
debugString?: string;
score?: number;
optimalValue: number | string;
extendedInfo?: {value: string};
notApplicable?: boolean;
error?: boolean;
// TODO: define details
details?: object;
Copy link
Member

Choose a reason for hiding this comment

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

(for when it's time to take on details...)

i did an initial spike of details's types in 95e19a5...typechecking#diff-518173330fe66b5974ceeb9a14a7b215

But then https://docs.google.com/document/d/1TumV5t3xtJ_e2b8y9PTwh4j2RaT-lmaQI9lwmF5tD2U/edit#heading=h.y9rlfi2u13u9 has a more future-forward definition, which is even stricter.

}

export interface AuditResults {
[metric: string]: AuditResult;
}

export interface AuditFullResult {
rawValue: boolean | number;
export interface AuditFullResult extends AuditResult {
displayValue: string;
debugString?: string;
score: number;
scoreDisplayMode: string;
error?: boolean;
scoreDisplayMode: 'numeric' | 'binary';
Copy link
Member

Choose a reason for hiding this comment

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

Audit.ScoringModes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Audit.ScoringModes is scoped to audit.js, so the externs would need to import Audit to have access. I think that's in our future anyways, but not worth it for this (it changes other things when our .d.ts file imports things because then it too becomes a module...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The future plan is to much as much as external facing stuff as possible to a .d.ts file anyway right?

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'm not sure. There's also what we want global vs scoped. What we produce for others to link against our public API may or may not be tied to that :)

Copy link
Member Author

@brendankenny brendankenny Mar 15, 2018

Choose a reason for hiding this comment

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

I think I was also wrong here. All the typedefs in Audit appear to be entirely local to the file and aren't exported, even though they're on an object that was, so all of these may need to be in d.ts files unless only used inside this file (that includes subclasses, so right now no audits can see the typedef of Audt.Meta even though they all import Audit)

description: string;
name: string;
helpText?: string;
extendedInfo?: {value: string};
informative?: boolean;
manual?: boolean;
}

export interface AuditFullResults {
Expand Down Expand Up @@ -140,3 +141,4 @@ export interface DevToolsJsonTarget {
url: string;
webSocketDebuggerUrl: string;
}
}