core(tsc): add type checking to gather-runner #4944

brendankenny · 2018-04-09T21:43:57Z

adding types was fairly straightforward.

I did have to async/await two functions: GatherRunner.afterPass (to get the compiler to realize all sorts of things aren't |undefined by the end of it) and GatherRunner.collectArtifacts (to isolate the type switcheroo from a dynamic collection of gatherer results to LH.Artifacts), but the translation was fairly mechanical and all existing tests were unchanged. They also look wayyy better than before :)

Also

split out an artifacts.d.ts file to start collecting artifact types
split out an config.d.ts file, not marking a difference between the JSON form that's expected as input to Config and the normalized type coming out of the other side (e.g. LH.Config.PassJson vs LH.Config.Pass). With any luck the second form won't be necessary when Config.js is typed

brendankenny · 2018-04-09T21:49:34Z

lighthouse-core/gather/gather-runner.js

      })
      .then(_ => driver.beginEmulation(options.settings))
      .then(_ => driver.enableRuntimeEvents())
      .then(_ => driver.cacheNatives())
      .then(_ => driver.registerPerformanceObserver())
      .then(_ => driver.dismissJavaScriptDialogs())
-      .then(_ => resetStorage && driver.clearDataForOrigin(options.url));
+      .then(_ => {
+        if (resetStorage) return driver.clearDataForOrigin(options.url);


sorry, tsc rightly complains about the return type of resetStorage && driver.clearDataForOrigin(options.url)). Good news is this will be much nicer when we switch to async/await for these functions :)

brendankenny · 2018-04-09T21:52:12Z

lighthouse-core/gather/gather-runner.js

    const tracingData = {
      traces: {},
      devtoolsLogs: {},
    };

-    if (typeof options.url !== 'string' || options.url.length === 0) {


these are already checked by runner, so no need to waste our time

brendankenny · 2018-04-09T21:52:40Z

lighthouse-core/test/gather/gather-runner-test.js

@@ -106,22 +106,12 @@ describe('GatherRunner', function() {
    });
  });

-  it('creates settings if needed', () => {


deleted tests are for deleted options.url, etc checks above

brendankenny · 2018-04-09T21:53:49Z

lighthouse-core/test/gather/gather-runner-test.js

@@ -622,6 +597,85 @@ describe('GatherRunner', function() {
  });

  describe('artifact collection', () => {
+    it('runs gatherer lifecycle methods strictly in sequence', async () => {


as we switch from promise reduce chains to for loops, want to make sure our gatherers never execute in parallel :)

sg. wanna add a comment saying this?

brendankenny · 2018-04-09T21:58:48Z

typings/artifacts.d.ts

+
+declare global {
+  module LH {
+    export interface Artifacts {


LH.Artifacts is both an interface and a namespace. Typescript correctly knows if you're referring to a type (e.g. @param {LH.Artifacts.TraceTimes} traceTimes in a jsdoc) to look up in the namespace while if you're referring to a property on something with the LH.Artifacts interface (e.g. artifacts.TraceTimes in actual js code) to look up in the interface.

Let me know if that's confusing, but in practice they're separate worlds and it doesn't seem to cause confusion. And we get to avoid a different name for each, e.g. LH.Artifacts.TraceTimes vs LH.ArtifactTypes.TraceTimes or whatever

brendankenny · 2018-04-09T22:02:19Z

lighthouse-cli/bin.js

 } else if (cliFlags.perf) {
-  config = /** @type {!LH.Config} */ (perfOnlyConfig);
+  config = perfOnlyConfig;


since these are now js files, typescript now imports them and checks that they're compatible with being assigned to LH.Config.Json|undefined here :)

paulirish · 2018-04-09T22:50:15Z

lighthouse-core/gather/gather-runner.js

    const gathererResults = {
-      LighthouseRunWarnings: [],
+      LighthouseRunWarnings: [[]],


What's up with this nested array? We always seem to only use [0] from it...

technically all gatherResults are nested arrays, we just tricked things before by nesting LighthouseRunWarnings at the last minute in collectArtifacts. Agreed that this version is a lot more fragile and (seemingly) arbitrary, though. I'll take another go at it

What's up with this nested array?

ok, how's that

paulirish · 2018-04-09T22:58:35Z

lighthouse-core/test/gather/gather-runner-test.js

@@ -622,6 +597,85 @@ describe('GatherRunner', function() {
  });

  describe('artifact collection', () => {
+    it('runs gatherer lifecycle methods strictly in sequence', async () => {


sg. wanna add a comment saying this?

paulirish · 2018-04-09T23:05:57Z

typings/config.d.ts

+       * The pre-normalization Lighthouse Config format.
+       */
+      export interface Json {
+        settings?: SettingsJson;


shall we flesh this out a bit more?

settings?; SettingsJson; passes?: PassJson[]; extends?: string; audits?: string[]; categories?: Object; groups?: Object;

audit might go down the rabbit hole that gatherers did but sg, it might make more sense to not type at all until we flesh them out fully though.

extends with 'lighthouse:full' | 'lighthouse:default' would be 🎯

yeah, I think we're going to have to face this when we add types to config.js and do it right

patrickhulce · 2018-04-09T23:23:32Z

typings/artifacts.d.ts

+  module LH {
+    export interface Artifacts {
+      fetchedAt: string;
+      LanternMetric: Artifacts.LanternMetric;


can remove this, TraceOfTab, and TraceTimes from this one, just need the typedefs

yeah, whoops, good catch. Computed artifacts.

patrickhulce

you know what they say, mo' types no problems! 👏 💯

patrickhulce · 2018-04-10T00:12:19Z

travis seems to be displeased about h2 though 🤔

brendankenny · 2018-04-10T01:05:23Z

weird. Travis now agrees that there's only 18 things loaded by dbw_tester, so good to go again

core(tsc): add type checking to gather-runner

53741e5

brendankenny requested review from patrickhulce and paulirish as code owners April 9, 2018 21:43

devtools-bot added the waiting4reviewer label Apr 9, 2018

brendankenny commented Apr 9, 2018

View reviewed changes

tweaks

fcf96a2

brendankenny commented Apr 9, 2018

View reviewed changes

paulirish reviewed Apr 9, 2018

View reviewed changes

brendankenny mentioned this pull request Apr 9, 2018

core(metrics): move TTCI to computed artifact #4943

Merged

patrickhulce reviewed Apr 9, 2018

View reviewed changes

brendankenny added 2 commits April 9, 2018 16:29

feedback

a4f55aa

make LighthouseRunWarnings not terrible

e290bc8

paulirish approved these changes Apr 10, 2018

View reviewed changes

patrickhulce approved these changes Apr 10, 2018

View reviewed changes

brendankenny merged commit 9823a65 into master Apr 10, 2018

brendankenny deleted the tsgatherrunner branch April 10, 2018 01:05

paulirish removed the waiting4reviewer label Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(tsc): add type checking to gather-runner #4944

core(tsc): add type checking to gather-runner #4944

brendankenny commented Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

paulirish Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

paulirish Apr 9, 2018

brendankenny Apr 9, 2018

brendankenny Apr 9, 2018

paulirish Apr 9, 2018

paulirish Apr 9, 2018

patrickhulce Apr 9, 2018 •

edited

Loading

brendankenny Apr 9, 2018

patrickhulce Apr 9, 2018

brendankenny Apr 9, 2018

patrickhulce left a comment

patrickhulce commented Apr 10, 2018

brendankenny commented Apr 10, 2018

core(tsc): add type checking to gather-runner #4944

core(tsc): add type checking to gather-runner #4944

Conversation

brendankenny commented Apr 9, 2018

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

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 Apr 9, 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

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce commented Apr 10, 2018

brendankenny commented Apr 10, 2018

patrickhulce Apr 9, 2018 •

edited

Loading