core(tsc): add type checking to runner #4961

brendankenny · 2018-04-13T00:10:55Z

also adds result.d.ts to type the full LHR and adds more types to config.d.ts (though it still needs a lot of work in combo with typing config.js).

brendankenny · 2018-04-13T00:13:48Z

lighthouse-core/audits/audit.js

+   * @param {LH.Audit.Context} context
+   * @return {LH.Audit.Product | Promise<LH.Audit.Product>}
+   */
+  static audit(artifacts, context) {


had to add this to the base class so typescript could see it in runner :)

brendankenny · 2018-04-13T00:30:24Z

lighthouse-core/audits/seo/robots-txt.js

+    // If status is good, content must be not null.
+    if (content === null) {
+      throw new Error(`Status ${status} was valid, but content was null`);
+    }


@kdzwinel had to add this to keep the type checker happy even though it appears to not be possible. I couldn't think of a good way around this short of adding another property that lets you demonstrate to the type checker that a null content can't happen in certain cases, e.g.
{error: false, status: number, content: string}|{error: true, status: number|null, content: string|null}
...or we can just test like here for // can't ever happen :)

brendankenny · 2018-04-13T00:31:18Z

runner.js is a lot easier to review at https://github.com/GoogleChrome/lighthouse/pull/4961/files?w=1

brendankenny · 2018-04-13T00:48:49Z

lighthouse-core/test/lib/asset-saver-test.js

@@ -27,21 +26,6 @@ function assertTraceEventsEqual(traceEventsA, traceEventsB) {

 /* eslint-env mocha */
 describe('asset-saver helper', () => {
-  it('generates HTML', () => {


tested by "screenshots html file saved to disk with data" below

brendankenny · 2018-04-13T00:49:28Z

lighthouse-core/test/lib/asset-saver-test.js

@@ -78,15 +61,15 @@ describe('asset-saver helper', () => {
      const ssHTMLFilename = 'the_file-0.screenshots.html';
      const ssFileContents = fs.readFileSync(ssHTMLFilename, 'utf8');
      assert.ok(/<!doctype/gim.test(ssFileContents));
-      const expectedScreenshotContent = '{"timestamp":674089419.919';
+      const expectedScreenshotContent = '{"timestamp":668545858.596';


now really pulling screenshots from the trace instead of having requestScreenshots resolve on the saved screenshots, which turned out weren't from this trace :)

brendankenny · 2018-04-13T00:50:08Z

lighthouse-core/test/runner-test.js

@@ -522,14 +520,10 @@ describe('Runner', () => {

    return Runner.run({}, {url, config}).then(results => {
      assert.deepEqual(results.artifacts.ViewportDimensions, ViewportDimensions);
-
-      for (const method of Object.keys(computedArtifacts)) {


artifacts from Runner.run() no longer include computed artifact methods on them. This and below just removes the check that they are on the returned artifacts object

brendankenny · 2018-04-13T00:50:41Z

lighthouse-core/scoring.js

@@ -16,14 +16,14 @@ const clampTo2Decimals = val => Math.round(val * 100) / 100;
 class ReportScoring {
  /**
   * Computes the weighted-average of the score of the list of items.
-   * @param {!Array<{score: number, weight: number|undefined}>} items
+   * @param {Array<{score: number, weight: number}>} items


turns out we always have score and weight by this point, so letting the type system handle it now

paulirish · 2018-04-13T00:13:52Z

lighthouse-core/audits/audit.js

+   *
+   * @param {LH.Artifacts} artifacts
+   * @param {LH.Audit.Context} context
+   * @return {LH.Audit.Product | Promise<LH.Audit.Product>}


satisfying to have this documented. :)

paulirish · 2018-04-13T00:30:31Z

typings/result.d.ts

+    /**
+     * The full output of a Lighthouse run.
+     */
+    export interface Result {


rename file lhr.d.t.s

paulirish · 2018-04-13T00:30:55Z

typings/result.d.ts

+     */
+    export interface Result {
+      /**
+       * The URL that was supplied to Lighthouse and initially navigated to.


use single line for these. :)

paulirish · 2018-04-13T00:31:20Z

typings/result.d.ts

+      /**
+       * Description of the runtime configuration used for gathering these results.
+       */
+      runtimeConfig: Result.RuntimeConfig;


lets add a bit more whitespace (and even a comment?) to separate lhr-non-lite

paulirish · 2018-04-13T00:40:29Z

lighthouse-core/test/scoring-test.js

@@ -30,15 +30,6 @@ describe('ReportScoring', () => {
        {score: 0.2, weight: 1},
      ]), 0.04);
    });
-
-    it('should work for missing values', () => {


yeah... :) we assert before here that things have a score so I agree with not worrying about this case.

paulirish · 2018-04-13T00:50:43Z

typings/config.d.ts

+      }
+
+      /**
+       * A LH Artifacts object, but the traces and devtoolsLogs are replaced


can u add a comment that this is present to support a legacy usecase. and link to the docs

was confused for a sec why runner artifacts were in here. :)

can we agree to nuke this use case before v3? it's a lot of config complexity for no known customers other than our tests

yup. we discussed yesterday that this is basically solved via
settings: { auditMode: 'path/to/artifacts/' }

patrick, that in line with your config/settings harmonization?

yup exactly 👍

brendankenny · 2018-04-13T00:52:49Z

lighthouse-core/runner.js

+   * @param {{config: LH.Config, url: string, initialUrl?: string, driverMock?: Driver}} opts
+   * @return {Promise<RunnerResult|undefined>}
+   */
+  static async run(connection, opts) {


opts now never leaves run() and _gatherArtifactsFromBrowser(), so we have a lot more freedom to do things like refactor our url/initialUrl/finalUrl business (not to mention options.settings (now isolated to _gatherArtifactsFromBrowser()) vs options.config.settings, etc)

very impressed with your commitment to appropriately back-ticking tokens in this sentence.
👨‍💻 ⌨️ 🥇

paulirish · 2018-04-13T00:52:19Z

typings/config.d.ts

+       * Reference to an audit member of a category and how its score should be
+       * weighted and how its results grouped with other members.
+       */
+      export interface CategoryMemberJson {


same as LH.Result.Category.CategoryMember

can we avoid defining it twice?

can we avoid defining it twice?

this is mostly mental organization that can be cleaned up when we add types to config.js. I'm not sure what we'll need by the end of things, so it was easier to keep separate for now.

Even if they end up exactly the same, we may still want two separate type names, one for acceptable input to config.js and one for what comes out. @patrickhulce mentions he uses something like typename and typenameUnsafe, and something like that might make it more obvious what the difference is. Though hopefully in our case you'll never see a *Json type inside of anything called by runner

okay gotcha. can we do a comment like

Object shape maintained all the way to LH.Result.Category.CategoryMember

oh, I guess that's the answer to why there's a LH.Config.CategoryMember and a LH.Config.CategoryMemberJson.

For why there's an identical LH.Result.CategoryMember, I'm not sure what the best thing to do is. We don't have to define it twice (or three times :), but it does mean that if we add a property to one and not the other in the future we can just do that (instead of splitting the definition later and having to figure out which one we meant wherever we refer to it). I'm also not sure which name should win.

added a comment and TODO

okay. I think the resolution here is that we ideally split Config and ConfigJSON into two .d.ts files. (and I'd favor a clearer module split for all the JSON buddies to keep them separate from what new Config() returns. \o/

paulirish · 2018-04-13T00:53:12Z

typings/config.d.ts

+        audits: CategoryMemberJson[];
+      }
+
+      export interface GroupJson {


same as LH.Result.Group

paulirish · 2018-04-13T00:59:10Z

typings/config.d.ts

+      }
+
+      // For now, these are unchanged from JSON version
+      export interface CategoryMember extends CategoryMemberJson {}


in addition to the doubled-up type (LH.Result.Category.CategoryMember), this has two names?

brendankenny · 2018-04-13T01:01:09Z

lighthouse-core/runner.js

-   * @param {!LH.ConfigSettings} settings
-   * @return {!Object} runtime config
+   * @param {LH.Config.Settings} settings
+   * @return {LH.Result.RuntimeConfig}


it's possible we want to just move this to the report generator now that we have more readable throttling information in config settings?

agree. not much left in this fn.

yeah, we should just output the settings used into the LHR :D

added to #4333

paulirish

Runner feels great. Nice!
I'm into the changes in scoring.

The instantiateComputedArtifacts change is quite nice too. As discussed, this sets up nicely to move our request* methods onto Audit and off artifacts in the future. :D

paulirish · 2018-04-13T01:03:07Z

lighthouse-core/runner.js

-  static _saveArtifacts(artifacts, path) {
-    return assetSaver.saveArtifacts(artifacts, path);
+    const driver = runnerOpts.driverMock || new Driver(connection);
+    const options = {


I see you already renamed opts to runnerOpts, though in the parent context is still has the opts name.

Regardless, feels like we can do gatherOpts for this guy. eviscerate all the options. woo!

patrickhulce

this is great! made a few comments, but overall lgtm

patrickhulce · 2018-04-13T16:35:09Z

lighthouse-core/lib/asset-saver.js

@@ -162,9 +162,13 @@ async function prepareAssets(artifacts, audits) {
  for (const passName of passNames) {
    const trace = artifacts.traces[passName];
    const devtoolsLog = artifacts.devtoolsLogs[passName];
+
+    // Avoid Runner->AssetSaver->Runner circular require by loading Runner here.


yeah, really disappointed to get there and run into that. Hopefully with computedArtifacts refactor that'll get cleaned up

patrickhulce · 2018-04-13T16:37:41Z

lighthouse-core/runner.js

+      try {
+        parsedURL = new URL(opts.url);
+      } catch (e) {
+        const err = new Error('The url provided should have a proper protocol and hostname.');


can probably just throw directly here

patrickhulce · 2018-04-13T16:45:08Z

lighthouse-core/runner.js

-   * @param {!LH.ConfigSettings} settings
-   * @return {!Object} runtime config
+   * @param {LH.Config.Settings} settings
+   * @return {LH.Result.RuntimeConfig}


yeah, we should just output the settings used into the LHR :D

patrickhulce · 2018-04-13T16:49:31Z

typings/config.d.ts

+      categories?: Record<string, Config.Category>;
+      groups?: Record<string, Config.Group>;
+      // TODO(bckenny): should be Partial<> maybe? don't want to fail imported json
+      artifacts?: Artifacts;


Partial sgtm now 👍

patrickhulce · 2018-04-13T16:50:26Z

typings/config.d.ts

+      }
+
+      /**
+       * A LH Artifacts object, but the traces and devtoolsLogs are replaced


can we agree to nuke this use case before v3? it's a lot of config complexity for no known customers other than our tests

patrickhulce · 2018-04-13T16:53:42Z

typings/lhr.d.ts

+      /**
+       * A description of configuration used for gathering.
+       */
+      export interface RuntimeConfig {


Yeah let's totally replace this with settings and have report renderer handle it

brendankenny · 2018-04-13T17:14:14Z

I'll follow up with the artifacts removal. Any other feedback?

brendankenny requested review from patrickhulce and paulirish as code owners April 13, 2018 00:10

brendankenny commented Apr 13, 2018

View reviewed changes

devtools-bot added the waiting4reviewer label Apr 13, 2018

brendankenny commented Apr 13, 2018

View reviewed changes

paulirish reviewed Apr 13, 2018

View reviewed changes

brendankenny commented Apr 13, 2018

View reviewed changes

paulirish requested changes Apr 13, 2018

View reviewed changes

brendankenny commented Apr 13, 2018

View reviewed changes

paulirish reviewed Apr 13, 2018

View reviewed changes

patrickhulce reviewed Apr 13, 2018

View reviewed changes

brendankenny added 5 commits April 13, 2018 10:17

core(tsc): add type checking to runner

203a584

lint and add test for notApplicable weighting

1834964

feedback

da5f9e6

add type TODO

d3ba187

throw the error directly

5cbd595

brendankenny force-pushed the tsrunner branch from 6ba9ba9 to 5cbd595 Compare April 13, 2018 17:18

brendankenny assigned paulirish Apr 13, 2018

paulirish approved these changes Apr 13, 2018

View reviewed changes

brendankenny merged commit cd2d36e into master Apr 13, 2018

brendankenny deleted the tsrunner branch April 13, 2018 17:56

paulirish removed the waiting4reviewer label Apr 30, 2018

brendankenny mentioned this pull request Aug 24, 2018

core: initial refactor of computedArtifact import/caching #5907

Merged

core(tsc): add type checking to runner #4961

core(tsc): add type checking to runner #4961

Conversation

brendankenny commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Apr 13, 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

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 Apr 13, 2018 • edited by paulirish 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

paulirish Apr 13, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 13, 2018

brendankenny Apr 13, 2018 •

edited

Loading

brendankenny Apr 13, 2018 •

edited by paulirish

Loading

paulirish Apr 13, 2018 •

edited

Loading