core(lhr): overhaul LHR details, introduce details.summary #4616

paulirish · 2018-02-23T19:06:29Z

Alright, #4384 laid the groundwork here and I've built off that for round two in the goal of "making the juicy details in LHR easy to consume."

(This is a super-breaking change. Yes.)

Changes

completely new shape of details:
- result.details.headings describes the expected report columns
- result.details.items contains row information, and may include more data than referenced by the headings. it's the new extendedInfo. :)
a new result.details.summary obj that includes whatever toplevel numeric summary we have. So far just wastedMs and wastedKb, but I could see displayValue migrating into it.

I think this is easiest to evaluate by looking at the LHR json. Here's one audit result, before and after:

🔎 Also here's a complete LHR: tinyhouse.json.txt

@patrickhulce @brendankenny Thoughts on the new LHR shape?

cc @benschwarz @denar90

This is a chain of PRs. Landing order is... 1st: newdetails (#4616). 2nd: shallowCategories (#4711). 3rd: scoring2.0 (#4690)

ref #4614

patrickhulce

🎉 ❤️ love the overall direction this is headed, I'd really like to nail down what details can look like in the new world and start documenting it as we go

Is this what we currently have?

interface AuditDetails {
  summary?: AuditDetailsSummary
  items?: AuditDetailsItem[]
  headings?: AuditTableHeading[]
  // ???
}

interface AuditDetailsItem {
  [key: string]: LegacyDetails | string | number
}

interface LegacyDetails {
  type: string
  value: string | number
  granularity?: string
  displayUnit?: string
}

interface AuditDetailsSummary {
  displayValue?: LegacyDetails
  wastedMs?: number // I might prefer sticking these two into a `performanceOpportunity` object or something, but we never really came up with other uses thus far either...
  wastedKb?: number
  // ???
}

interface AuditTableHeading extends LegacyDetails {
  key: string
  text: string
}

patrickhulce · 2018-02-23T19:13:57Z

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

@@ -315,7 +337,7 @@ class DetailsRenderer {
   */
  _renderCode(details) {
    const pre = this._dom.createElement('pre', 'lh-code');
-    pre.textContent = details.text;
+    pre.textContent = details.text || details.value; // TODO, pick a winner


+1 for value

patrickhulce · 2018-02-23T19:15:37Z

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

-      return this._dom.createElement('span');
-    }
+  _renderThumbnail(details) {
+    // TODO: use some empty details test instead? to handle data uris?


yeah let's strip out all data URIs at the audit base-class, they can make the report super beefy anyway

patrickhulce · 2018-02-23T19:16:49Z

lighthouse-core/report/v2/renderer/performance-category-renderer.js

-    const extendedInfo = /** @type {!PerformanceCategoryRenderer.PerfHintExtendedInfo}
-        */ (audit.result.extendedInfo);
+    const summaryInfo = /** @type {!PerformanceCategoryRenderer.PerfHintSummaryInfo}
+        */ (audit.result.summary);


I might be missing something, I thought we were nesting summary inside of details?

denar90 · 2018-02-25T20:48:46Z

Wow, big amount of work! Thanks for noticing 👍
Is there any v3 roadmap issue? I just found only 2 pr's but no doubt there will be more)

patrickhulce · 2018-02-26T17:07:34Z

@denar90 #4333 is our wishlist for v3 breaking changes :)

paulirish · 2018-02-27T22:32:48Z

Is this what we currently have?

Yes, that looks mostly accurate. I'm almost done with typechecking the JSON against an iteration of these interfaces. ;)

In the case of PSI audits we have an even simpler details story:

interface AuditDetails {
  summary?: AuditDetailsSummary  // could be also be a sibling to details..
  headings?: AuditTableHeading[]
  items?: AuditDetailsItem[]
}

interface AuditDetailsSummary {
  wastedMs?: number // maybe these two within a `performanceOpportunity` object?
  wastedKb?: number
}

interface AuditTableHeading {
  key: string
  text: string
  itemType: string
  granularity?: string
  displayUnit?: string
}

interface AuditDetailsItem {
  [key: string]: string | number
}

paulirish · 2018-03-02T22:18:28Z

@patrickhulce this is ready for review.

patrickhulce · 2018-03-02T23:26:17Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

@@ -18,7 +18,7 @@ const IGNORE_THRESHOLD_IN_PERCENT = 0.925;
 const SCORING_POINT_OF_DIMINISHING_RETURNS = 4; // 4 KB
 const SCORING_MEDIAN = 768; // 768 KB

-class CacheHeaders extends ByteEfficiencyAudit {
+class CacheHeaders {


@paulirish let's make sure this extends audit

patrickhulce · 2018-03-02T23:28:35Z

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

@@ -133,11 +137,16 @@ class DetailsRenderer {
   * @return {!Element}
   */
  _renderLink(details) {
+    console.assert(('text' in details), 'link details must have .url');


seems like this should be 'url' in details?

patrickhulce · 2018-03-02T23:29:34Z

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

+      for (const heading of details.headings) {
+        const value = /** @type {number|string|!DetailsRenderer.DetailsJSON} */ (row[heading.key]);
+        if (typeof value === 'undefined') {
+          continue; // e.g. no lineNumber in this row item


don't we still want to create a td element even if it's empty?

patrickhulce · 2018-03-02T23:32:29Z

lighthouse-core/runner.js

@@ -93,9 +93,10 @@ class Runner {
      const resultsById = {};
      for (const audit of runResults.auditResults) resultsById[audit.name] = audit;

-      let report;
+      let cats;


can we go categories though :)

paulirish · 2018-03-02T23:58:34Z

updated and squashed (to resolve silly conflicts)

…w summary * completely new shape of `details`: * `result.details.headings` describes the expected report columns * `result.details.items` contains row information, and may include _more_ data than referenced by the headings. it's the new `extendedInfo`. :) * new `result.details.summary` obj that includes whatever toplevel numeric summary we have. So far just `wastedMs` and `wastedKb` * `reportCategories` not contain real auditResults (aka _de-dupe all audit results_ from #4333) * `reportCategories.performance` keeps it's `score` prop * each audit's score moves from `reportCategories.performance['audit-name']` (which now just holds `weight` and `group`) over to `audits['audit-name']` * removes `LHR.score`, the overallScore

paulirish · 2018-03-07T21:46:03Z

@patrickhulce lgty?

patrickhulce

I say merge away! Never a better time for the risky business than now 👍

brendankenny

is it possible to take out most of these runtime details checks and rely on the type checker(s)? e.g. at DetailsRenderer.render, if type is text, we should be certain that it's a StringDetailsJSON at that point. We can do some of this by checking the output of our audits, but some of it should fall out naturally…

also, it looks like we won’t be able to non-awkwardly be able to get read of text.text since so many other details types still include a text?

brendankenny · 2018-03-07T21:56:07Z

lighthouse-core/audits/audit.js

@@ -141,20 +114,35 @@ class Audit {
    if (displayValue === score) {
      displayValue = '';
    }
+
+    // TODO: restore after initial 3.0 branching


brendankenny · 2018-03-07T21:56:50Z

lighthouse-core/audits/audit.js

+
+    // TODO, don't consider an auditResult's scoringMode (currently applied to all ByteEfficiency)
+    const scoringMode = result.scoringMode || audit.meta.scoringMode || Audit.SCORING_MODES.BINARY;
+    delete result.scoringMode;


add note that will be removed in next PR

brendankenny · 2018-03-07T21:58:49Z

lighthouse-core/audits/byte-efficiency/uses-long-cache-ttl.js

@@ -7,6 +7,7 @@

 const assert = require('assert');
 const parseCacheControl = require('parse-cache-control');
+const Audit = require('../audit');
 const ByteEfficiencyAudit = require('./byte-efficiency-audit');


still need ByteEfficiencyAudit? Looks like most things might just be through Audit?

brendankenny · 2018-03-07T22:01:55Z

lighthouse-core/report/v2/renderer/category-renderer.js

@@ -203,9 +203,10 @@ class CategoryRenderer {
    const auditsGroupedByGroup = /** @type {!Object<string,
        !Array<!ReportRenderer.AuditJSON>>} */ ({});
    manualAudits.forEach(audit => {
-      const group = auditsGroupedByGroup[audit.group] || [];
+      const groupId = audit.group || 'ungrouped';


what is this doing?

brendankenny · 2018-03-07T22:04:17Z

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

   * @return {!Element}
   */
  _renderTextURL(text) {
-    const url = text.text || '';
+    console.assert(!('text' in text), 'url details should not have .text');
+    console.assert('value' in text, 'url details must have .value');


these should be checked by the type system?

brendankenny · 2018-03-07T22:04:48Z

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

@@ -133,11 +137,16 @@ class DetailsRenderer {
   * @return {!Element}
   */
  _renderLink(details) {
+    console.assert(('url' in details), 'link details must have .url');
+    console.assert(('text' in details), 'link details must have .text');


same as above

brendankenny · 2018-03-07T22:07:46Z

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

+ *     wastedBytes: (number|undefined),
+ * }}
+ */
+DetailsRenderer.DetailsSummary; // eslint-disable-line no-unused-expressions


this seems more specific than a just a "summary"?

brendankenny · 2018-03-07T22:35:17Z

if we want to get really fancy (and if we do TS checking of the report renderer), we can use discriminated unions which will type check based on the value of type in a particular branch. Will need to be in an externs file, though, because closure will choke on type: "text" in the typedef

paulirish · 2018-03-08T22:06:08Z

also, it looks like we won’t be able to non-awkwardly be able to get read of text.text since so many other details types still include a text?

fwiw the current shape of details objects looks like this:

detail types with 1 required field

type text with .value
type code with .value
type thumbnail w/ .value

type ms with .value
type bytes w .value
type url w .value

detail types with >1 required field

type link with .text and .url
type node with .snippet .selector .path
type card with .title .value .target .snippet

Additionally, the second block of 1-field types (ms/bytes/url) are currently never expressed in the LHR explicitly; they are all created via headings..itemType.

brendankenny

LGTM

- completely new shape of `details`: - `result.details.headings` describes the expected report columns - `result.details.items` contains row information, and may include _more_ data than referenced by the headings. it's the new `extendedInfo`. :) - new `result.details.summary` obj that includes whatever toplevel numeric summary we have. So far just `wastedMs` and `wastedKb`

paulirish added internals needs-discussion labels Feb 23, 2018

devtools-bot added the waiting4reviewer label Feb 23, 2018

paulirish added the 3.0 label Feb 23, 2018

This was referenced Feb 23, 2018

Improve LHR API shape for downstream consumers #4614

Closed

core: improved consumption of audit.details #4384

Merged

patrickhulce reviewed Feb 23, 2018

View reviewed changes

patrickhulce mentioned this pull request Feb 28, 2018

core(network-requests): add network requests audit #4631

Merged

Mihail15 approved these changes Mar 2, 2018

View reviewed changes

paulirish changed the title ~~WIP: refactor LHR shape of details/extendedInfo~~ core: refactor LHR shape: scriptable details, clean reportCategories, new summary Mar 2, 2018

paulirish changed the title ~~core: refactor LHR shape: scriptable details, clean reportCategories, new summary~~ core: consumable LHR - scriptable details, clean reportCategories, new summary Mar 2, 2018

patrickhulce suggested changes Mar 2, 2018

View reviewed changes

paulirish changed the base branch from (╯°□°）╯︵┻━┻_the_strings to master March 2, 2018 23:47

paulirish force-pushed the newdetails branch 2 times, most recently from 27f425c to c0658ec Compare March 2, 2018 23:57

paulirish force-pushed the newdetails branch 2 times, most recently from fa7407c to 7486a4a Compare March 3, 2018 01:25

patrickhulce mentioned this pull request Mar 3, 2018

☂️ 👣 Things to fix on next breaking change #4333

Closed

34 tasks

paulirish force-pushed the newdetails branch from 7486a4a to a594d44 Compare March 3, 2018 04:10

paulirish changed the title ~~core: consumable LHR - scriptable details, clean reportCategories, new summary~~ core(LHR): overhaul details, incl details.summary, shallow reportCategories Mar 7, 2018

paulirish changed the title ~~core(LHR): overhaul details, incl details.summary, shallow reportCategories~~ core(lhr): overhaul details, incl details.summary, shallow reportCategories Mar 7, 2018

patrickhulce approved these changes Mar 7, 2018

View reviewed changes

brendankenny reviewed Mar 7, 2018

View reviewed changes

extract reportCategories/score changes

72c8a6e

paulirish force-pushed the newdetails branch from 849eb7d to 72c8a6e Compare March 7, 2018 23:08

This was referenced Mar 8, 2018

core(lhr): strictly numeric scores, add scoreDisplayMode #4690

Merged

core(lhr): make reportCategories shallow; move audit scores to AuditResultJSON #4711

Merged

paulirish changed the title ~~core(lhr): overhaul details, incl details.summary, shallow reportCategories~~ core(lhr): overhaul LHR details, introduce details.summary Mar 8, 2018

paulirish added 2 commits March 8, 2018 13:40

address brendan's feedback.

eb9018e

closure.

bfe9d71

paulirish force-pushed the newdetails branch from 354bc15 to bfe9d71 Compare March 8, 2018 21:55

brendankenny approved these changes Mar 9, 2018

View reviewed changes

paulirish merged commit bc9efba into master Mar 9, 2018

denar90 mentioned this pull request Mar 10, 2018

Update LH dep to v3 paulirish/pwmetrics#158

Closed

paulirish removed the waiting4reviewer label Mar 15, 2018

This was referenced Mar 20, 2018

Report: empty details table row when showing URLs #4818

Closed

core(is-crawlable): fix empty row in the details table when showing an URL #4820

Merged

paulirish deleted the newdetails branch April 27, 2018 21:51

paulirish removed the 3.0 label Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lhr): overhaul LHR details, introduce details.summary #4616

core(lhr): overhaul LHR details, introduce details.summary #4616

paulirish commented Feb 23, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading

patrickhulce Feb 23, 2018

patrickhulce Feb 23, 2018

patrickhulce Feb 23, 2018

denar90 commented Feb 25, 2018

patrickhulce commented Feb 26, 2018

paulirish commented Feb 27, 2018

paulirish commented Mar 2, 2018

patrickhulce Mar 2, 2018

patrickhulce Mar 2, 2018

patrickhulce Mar 2, 2018

patrickhulce Mar 2, 2018

paulirish commented Mar 2, 2018

paulirish commented Mar 7, 2018

patrickhulce left a comment

brendankenny left a comment

brendankenny Mar 7, 2018

brendankenny Mar 7, 2018

brendankenny Mar 7, 2018 •

edited

Loading

brendankenny Mar 7, 2018

brendankenny Mar 7, 2018

brendankenny Mar 7, 2018

brendankenny Mar 7, 2018

brendankenny commented Mar 7, 2018

paulirish commented Mar 8, 2018

brendankenny left a comment

core(lhr): overhaul LHR details, introduce details.summary #4616

core(lhr): overhaul LHR details, introduce details.summary #4616

Conversation

paulirish commented Feb 23, 2018 • edited Loading

Changes

patrickhulce left a comment • 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

denar90 commented Feb 25, 2018

patrickhulce commented Feb 26, 2018

paulirish commented Feb 27, 2018

paulirish commented Mar 2, 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

paulirish commented Mar 2, 2018

paulirish commented Mar 7, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 7, 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

brendankenny commented Mar 7, 2018

paulirish commented Mar 8, 2018

detail types with 1 required field

detail types with >1 required field

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Feb 23, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading

brendankenny Mar 7, 2018 •

edited

Loading