core: add type checking to audit and gatherer base classes #4762

brendankenny · 2018-03-14T20:37:21Z

having typing here helps in a number of places and was easy to pull out since they're relatively isolated. This is just the base classes...all the implementing classes are untouched (and unchecked)

brendankenny · 2018-03-14T20:39:31Z

lighthouse-core/audits/audit.js

-   * @return {!DetailsRenderer.DetailsJSON}
+   * @param {Audit.Headings} headings
+   * @param {Array<Object<string, string>>} results
+   * @param {Audit.DetailsRenderer.DetailsSummary} summary


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 :)

brendankenny · 2018-03-14T20:41:02Z

typings/externs.d.ts

-  type: string;
-  url: string;
-  webSocketDebuggerUrl: string;
+declare namespace LH {


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.

https://github.com/GoogleChrome/lighthouse/pull/4762/files?w=1#diff-518173330fe66b5974ceeb9a14a7b215 to see actual changes to externs file

patrickhulce

nice 👍

patrickhulce · 2018-03-14T20:41:34Z

lighthouse-core/gather/gatherers/gatherer.js

   */
-  beforePass(options) { }
+  beforePass(passContext) { }


patrickhulce · 2018-03-14T20:41:55Z

lighthouse-core/gather/gatherers/gatherer.js


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

+/**
+ * @typedef {Object} Gatherer.PassContext


patrickhulce · 2018-03-14T20:42:12Z

tsconfig.json

@@ -18,6 +18,7 @@
    "lighthouse-cli/**/*.js",
    "lighthouse-core/lib/dependency-graph/**/*.js",
    "lighthouse-core/gather/connections/**/*.js",
+    "lighthouse-core/gather/gatherers/gatherer.js",


add audit.js too?

yeah, better to be explicit

paulirish

this is looking nice!

paulirish · 2018-03-14T20:53:35Z

lighthouse-core/audits/audit.js

@@ -65,9 +64,9 @@ class Audit {
  }

  /**
-   * @param {!Audit} audit
+   * @param {typeof Audit} audit


what's this mean btw?

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...

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

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... 🤪

paulirish · 2018-03-14T20:57:11Z

lighthouse-core/audits/audit.js

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


we could go with this for now:

wastedMs?: number wastedKb?: number

paulirish

a few questions below.

also, policy-wise how do we want to handle externs vs typedefs within the source file? and can we avoid duplicating the interfaces between the two?

paulirish · 2018-03-14T22:12:43Z

lighthouse-core/audits/audit.js

+ * @property {string} description
+ * @property {string} helpText
+ * @property {Array<string>} requiredArtifacts
+ * @property {string=} failureDescription


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.

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

k can we do foo|undefined?

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

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

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

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

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

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

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 :)

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.

paulirish · 2018-03-14T22:14:49Z

lighthouse-core/audits/audit.js

-   * @param {!AuditResult} result
-   * @return {!AuditFullResult}
+   * @param {typeof Audit} audit
+   * @param {LH.AuditResult} result


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

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

fine with punting for today.

paulirish · 2018-03-14T22:17:19Z

typings/externs.d.ts

+  export interface AuditFullResult extends AuditResult {
+    displayValue: string;
+    score: number;
+    scoreDisplayMode: 'numeric' | 'binary';


Audit.ScoringModes?

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...)

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

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 :)

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)

paulirish · 2018-03-14T22:18:01Z

lighthouse-core/audits/audit.js

-   * @return {{score: number, scoreDisplayMode: string}}
+   * @param {typeof Audit} audit
+   * @param {LH.AuditResult} result
+   * @return {{score: number, scoreDisplayMode: Audit.ScoringModeValues}}


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

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

paulirish · 2018-03-14T22:25:05Z

typings/externs.d.ts

+    notApplicable?: boolean;
+    error?: boolean;
+    // TODO: define details
+    details?: object;


(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.

paulirish · 2018-03-14T22:25:14Z

typings/externs.d.ts

+  export interface Config {}
+
+  export interface AuditResult {
+    rawValue: boolean | number | null;


we need null?

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

mmmmmmmmmmmkkkkkkkkkkkkkkkkkkkkayyyyyyyyyyy.

paulirish · 2018-03-14T23:06:46Z

lgtm once foo|undefined

This reverts commit b8e22e2.

brendankenny · 2018-03-15T21:43:29Z

fixed up optional properties. I'll follow up with d.ts changes later

brendankenny requested review from patrickhulce and paulirish as code owners March 14, 2018 20:37

brendankenny commented Mar 14, 2018

View reviewed changes

patrickhulce reviewed Mar 14, 2018

View reviewed changes

core: add type checking to audit and gatherer base classes

7bf9705

brendankenny force-pushed the tsag branch from c5b629c to 7bf9705 Compare March 14, 2018 20:43

devtools-bot added the waiting4reviewer label Mar 14, 2018

explicitly check audit.js

0401a79

paulirish reviewed Mar 14, 2018

View reviewed changes

dummy summary

7dd1240

paulirish requested changes Mar 14, 2018

View reviewed changes

brendankenny added 3 commits March 14, 2018 23:21

|undefined

b8e22e2

Revert "|undefined"

ddb7f5b

This reverts commit b8e22e2.

[]

22fcbfe

paulirish approved these changes Mar 15, 2018

View reviewed changes

brendankenny merged commit 7c7695b into master Mar 15, 2018

brendankenny deleted the tsag branch March 15, 2018 22:31

paulirish removed the waiting4reviewer label Apr 30, 2018

core: add type checking to audit and gatherer base classes #4762

core: add type checking to audit and gatherer base classes #4762

Conversation

brendankenny commented Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 14, 2018

brendankenny commented Mar 15, 2018

brendankenny commented Mar 14, 2018 •

edited

Loading

brendankenny Mar 14, 2018 •

edited

Loading

brendankenny Mar 14, 2018 •

edited

Loading

brendankenny Mar 14, 2018 •

edited

Loading

patrickhulce Mar 15, 2018 •

edited

Loading

brendankenny Mar 15, 2018 •

edited

Loading