core(lhr): migrate opportunity details to new format #5296

brendankenny · 2018-05-22T01:06:35Z

part of #5008

Moves opportunity details over to the new format. Wanted to get feedback before changing the many tests that are failing due to the slightly different details shape.

Most of the byte efficiency audit changes are just name changes, e.g. results->items, itemType->valueType.

Shape of the new opportunity details is what's in lhr-lite:

lighthouse/typings/lhr-lite.d.ts

Lines 85 to 91 in 804e9b8

    
           export interface OpportunityDetails { 
        
             type: 'opportunity'; 
        
             overallSavingsMs: number 
        
             overallSavingsBytes?: number 
        
             headings: ColumnHeading[]; 
        
             items: (WastedBytesDetailsItem | WastedTimeDetailsItem)[]; 
        
           }

with the addition of more possibilities in OpportunityDetailsItem than is in the more limited lhr-lite.

brendankenny · 2018-05-22T01:08:30Z

lighthouse-core/audits/audit.js

+   * @param {number=} overallSavingsBytes
+   * @return {LH.Result.Audit.OpportunityDetails}
+   */
+  static makeOpportunityDetails(headings, items, overallSavingsMs, overallSavingsBytes) {


there's no real reason this needs to be a thing, we just need a place to check that audits are returning the correct shape. If we start enforcing the return type of audits based on the details they provide (e.g. @return {LH.Audit.Result<OpportunityDetails>} or whatever) that could work just as well.

brendankenny · 2018-05-22T01:12:24Z

lighthouse-core/report/html/renderer/details-renderer.js

@@ -242,6 +244,79 @@ class DetailsRenderer {
    return tableElem;
  }

+  /**


this is called _renderOpportunityTable for now, but I'd like to migrate all audits with table details to this one and eventually just have it be _renderTable. The changes from the original mean we only have so many types of things that can fit in the table cells, so just call them out explicitly instead of having nested details that can themselves contain more details, etc.

E.g. here we get rid of the "URLs" that are actually {type: 'code', value: txt}, and instead just say if it doesn't look like a URL, render it in <pre></pre> tags

I like this. Does this mean we'd eliminate the fallbackType/fallbackKey approach? Also this method feels a bit long, want to break it up? :)

I like this. Does this mean we'd eliminate the fallbackType/fallbackKey approach?

I guess it comes down to how flexible we need it to end up being. It doesn't seem like we have a ton of cases where we need fallbacks, so we can probably get by with using hybrid valueTypes, like url is here (maybe we'd want to rename them based on their actual combined type? e.g. 'urlOrCode', but something better :)

can you add a comment with this todo?

patrickhulce

I think this all looks good to me! all seems to be in line with what we planned other than the fallback* issue, but I like the new version if we can get away with it

patrickhulce · 2018-05-22T16:30:16Z

lighthouse-core/report/html/renderer/details-renderer.js

      case 'node':
        return this.renderNode(/** @type {NodeDetailsJSON} */(details));
      case 'criticalrequestchain':
        return CriticalRequestChainRenderer.render(this._dom, this._templateContext,
          // @ts-ignore - TODO(bckenny): Fix type hierarchy
          /** @type {CRCDetailsJSON} */ (details));
+      case 'opportunity':
+        // @ts-ignore - TODO(bckenny): Fix type hierarchy
+        return this._renderTable(/** @type {OpportunityDetails} */ (details));


render opportunity table?

patrickhulce · 2018-05-22T16:32:59Z

lighthouse-core/test/results/sample_v2.json

-        "type": "table",
-        "headings": [],
+        "type": "opportunity",
+        "headings": [


seems like we still don't want to populate headings if there are no items

seems like we still don't want to populate headings if there are no items

makes me sad but ok

patrickhulce · 2018-05-22T16:33:55Z

typings/lhr.d.ts

+          totalBytes?: number;
+          wastedMs?: number;
+          wastedPercent?: number;
+          fromProtocol?: boolean;


do we want to just nuke these?

do we want to just nuke these?

yeah, I was building them up to make sure every case was covered, but probably should just be an index type

but probably should just be an index type

turns out it wasn't that simple because we want them to extend lhr-lite, but now an index type + minimum set of properties

paulirish

seems good. a few questions

paulirish · 2018-05-22T19:01:39Z

typings/lhr.d.ts

@@ -81,6 +81,23 @@ declare global {
        blockedUrlPatterns: string[];
        extraHeaders: Crdp.Network.Headers;
      }
+
+      export module Audit {


curious why this is in lhr.d.ts instead of audit.d.ts

curious why this is in lhr.d.ts instead of audit.d.ts

Like lhr-lite.d.ts, it seems like all the LHR stuff should be defined in lhr.d.ts, instead of spread over lhr.d.ts and audit.d.ts. That would leave audit.d.ts with types internal to operating an audit, while Audit.Result stuff would all be in lhr.d.t.s (with a good chunk of that just inherited from lhr-lite.d.ts).

This starts on that path, plus is a nice dividing line between this finalized-ish details definition vs the handwavey/just-make-it-work definitions in audit.d.ts

paulirish · 2018-05-22T19:04:29Z

lighthouse-core/report/html/renderer/details-renderer.js

@@ -242,6 +244,79 @@ class DetailsRenderer {
    return tableElem;
  }

+  /**


can you add a comment with this todo?

paulirish · 2018-05-22T19:07:14Z

lighthouse-core/report/html/renderer/details-renderer.js

      default: {
        throw new Error(`Unknown type: ${details.type}`);
      }
    }
  }

  /**
-   * @param {NumericUnitDetailsJSON} details
+   * @param {{value: number, granularity?: number}} details


bytes defaults to granularity of 0.1 in util, so removing the granularity in the audits means these will now show as more precise, right?

dont' we want to keep the granularity the same?

bytes defaults to granularity of 0.1 in util, so removing the granularity in the audits means these will now show as more precise, right? dont' we want to keep the granularity the same?

I forget what we decided on granularity for these. Always to 1 or was bytes going to be one of the auto-formatting options?

happy with defaulting opportunity bytes granularity to 1.

brendankenny · 2018-05-22T21:57:09Z

turns out tests weren't bad, mostly s/results/items and flattening the smoke expectations a bit. Ready for really real review

patrickhulce

mostly just the extra headings in the JSON thing, other than that looks good to me!

patrickhulce · 2018-05-23T22:25:00Z

lighthouse-core/report/html/renderer/details-renderer.js

@@ -55,31 +59,34 @@ class DetailsRenderer {
        // @ts-ignore - TODO(bckenny): Fix type hierarchy
        return this._renderTable(/** @type {TableDetailsJSON} */ (details));
      case 'code':
-        return this._renderCode(details);
+        return this._renderCode(/** @type {StringDetailsJSON} */ (details));


if we're killing string details json elsewhere should we be moving this to it?

if we're killing string details json elsewhere should we be moving this to it?

I can leave it as DetailsJSON? Basically I need to tell it that it's not an OpportunityDetails, and until all the details types are known to the compiler with type defined it isn't narrowing it down here

Oh I meant elsewhere it seems like we're deleting StringDetailsJSON and going with literal {text: string} typedefs, so should we just do that instead

patrickhulce · 2018-05-23T22:27:02Z

lighthouse-core/report/html/report-styles.css

@@ -842,6 +842,7 @@ summary.lh-passed-audits-summary {

 .lh-table-column--text,
 .lh-table-column--bytes,
+.lh-table-column--timespanMs,


😢 I guess a _.toKebabCase is a bit much huh?

I guess a _.toKebabCase is a bit much huh?

ha, that's a good point, but it'd also be pretty ugly. Is the tear because you're accepting living with it or do you really want this?

💦 because I'm living with it :)

patrickhulce · 2018-05-23T22:27:45Z

lighthouse-core/test/audits/byte-efficiency/unused-css-rules-test.js

-      const expectedPreview = {type: 'code', value: 'dummy'};
-      assert.deepEqual(map({header: {sourceURL: ''}}).url, expectedPreview);
-      assert.deepEqual(map({header: {sourceURL: 'a'}}, 'http://g.co/a').url, expectedPreview);
+      const expectedPreview ='dummy';


nit: space after =

what are you doing eslint

patrickhulce · 2018-05-23T22:28:00Z

lighthouse-core/test/results/sample_v2.json

-        "type": "table",
-        "headings": [],
+        "type": "opportunity",
+        "headings": [


brendankenny · 2018-05-24T19:31:26Z

updated

patrickhulce

LGTM! 💯 🆕

patrickhulce · 2018-05-24T20:48:24Z

lighthouse-core/report/html/renderer/details-renderer.js

+   * @param {OpportunityDetails} details
+   * @return {Element}
+   */
+  _renderOpportunityTable(details) {


this fn is a bit long, you foresee breaking it up in the future? add a TODO if so :)

paulirish · 2018-05-25T00:15:37Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

-      {key: 'wastedMs', itemType: 'ms', text: 'Download Time (ms)', granularity: 1},
+      {key: 'url', valueType: 'url', label: 'URL'},
+      {key: 'totalBytes', valueType: 'bytes', label: 'Size (KB)'},
+      {key: 'wastedMs', valueType: 'timespanMs', label: 'Download Time (ms)'},


we'll be dropping granularity of 1 and getting default of 10 afaict.
that is probably fine, but just FYI

brendankenny requested review from patrickhulce, paulirish and vinamratasingal-zz as code owners May 22, 2018 01:06

paulirish deployed to PR staging May 22, 2018 01:06 View deployment

brendankenny commented May 22, 2018

View reviewed changes

paulirish deployed to PR staging May 22, 2018 01:19 View deployment

patrickhulce reviewed May 22, 2018

View reviewed changes

paulirish reviewed May 22, 2018

View reviewed changes

paulirish deployed to PR staging May 22, 2018 19:17 View deployment

paulirish deployed to PR staging May 22, 2018 19:24 View deployment

paulirish deployed to PR staging May 22, 2018 19:45 View deployment

paulirish deployed to PR staging May 22, 2018 21:45 View deployment

paulirish deployed to PR staging May 22, 2018 21:54 View deployment

brendankenny changed the title ~~WIP: core(lhr): migrate opportunity details to new format~~ core(lhr): migrate opportunity details to new format May 22, 2018

paulirish deployed to PR staging May 22, 2018 22:41 View deployment

brendankenny requested review from paulirish and patrickhulce May 23, 2018 22:22

patrickhulce suggested changes May 23, 2018

View reviewed changes

brendankenny added 10 commits May 24, 2018 12:22

core(lhr): migrate opportunity details to new format

b1420fb

update golden LHR for preview of changes

0762da4

feedback

4b0c456

eliminate excess type decls

9f4d57a

whoops

78b5aef

feedback

4b2ce68

fix unit tests

6cac8fc

fix smoke tests

c155ccc

fix opportunity byte granularity

788ad28

more feedback

7c42100

brendankenny force-pushed the opps branch from f0a69d2 to 7c42100 Compare May 24, 2018 19:31

paulirish deployed to PR staging May 24, 2018 19:31 View deployment

patrickhulce approved these changes May 24, 2018

View reviewed changes

add TODO

f4ff217

paulirish deployed to PR staging May 24, 2018 21:16 View deployment

paulirish approved these changes May 25, 2018

View reviewed changes

brendankenny merged commit 8f500e0 into master May 25, 2018

brendankenny deleted the opps branch May 25, 2018 00:37

paulirish mentioned this pull request Jun 25, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

paulirish mentioned this pull request Aug 6, 2018

core(redirects): update to modern opportunity details type #5791

Merged

	export interface OpportunityDetails {
	type: 'opportunity';
	overallSavingsMs: number
	overallSavingsBytes?: number
	headings: ColumnHeading[];
	items: (WastedBytesDetailsItem \| WastedTimeDetailsItem)[];
	}

core(lhr): migrate opportunity details to new format #5296

core(lhr): migrate opportunity details to new format #5296

Conversation

brendankenny commented May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 22, 2018 • edited Loading

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

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

brendankenny commented May 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented May 24, 2018

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

brendankenny commented May 22, 2018 •

edited

Loading

patrickhulce May 22, 2018 •

edited

Loading