core: remove uses of deprecated extendedInfo field #10779

Beytoven · 2020-05-14T01:09:33Z

This PR removes the remaining usage of 'extendedInfo' which has been deprecated for a while. For each of the audits, extendedInfo was moved into debugData if it hasn't been replicated in the audit details already.

Closes #10567

paulirish

nice stuff @Beytoven

i went through each use and verified that the data is either already in details or its worthwhile to expose in debugdata.

there's a few i'm not sure about so I left comments for folks to chime in on.

paulirish · 2020-05-15T18:39:50Z

lighthouse-core/audits/metrics/interactive.js

@@ -74,7 +74,9 @@ class InteractiveMetric extends Audit {
    const timeInMs = metricResult.timing;
    const isDesktop = artifacts.TestedAsMobileDevice === false;
    const options = isDesktop ? context.options.desktop : context.options.mobile;
-    const extendedInfo = {
+    /** @type {LH.Audit.Details.DebugData} */
+    const debugData = {


do we still need this? cc @patrickhulce

in samplejson it ends up with just timeinMs and timestamp which are both already in 'metrics' audit.

not sure if we want the optimisitic/pessimistic data tho.

Nope, let's dump it! ✂️

paulirish · 2020-05-15T18:42:08Z

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

+      /** @type {LH.Audit.Details.DebugData} */
+      const debugData = {
+        type: 'debugdata',
+        queryStringCount,


do we still want this? original context: #3531 (comment)

cc @brendankenny @patrickhulce

given that it hasnt come up since then i'm prone to exclude it

+1 to dropping

paulirish · 2020-05-15T18:44:13Z

lighthouse-core/audits/byte-efficiency/total-byte-weight.js

+    /** @type {LH.Audit.Details.DebugData} */
+    const debugData = {
+      type: 'debugdata',
+      totalCompletedRequests,


original context: #2697

we added the network-requests audit since then so we don't need this any longer.

paulirish · 2020-05-15T18:45:35Z

lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js

+    const debugData = {
+      type: 'debugdata',
+      jsLibs: libraryVulns,
+      vulnerabilityResults,


we don't need vulnerabilityResults here because its already in details.

paulirish · 2020-05-15T18:52:52Z

lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js

+    /** @type {LH.Audit.Details.DebugData} */
+    const debugData = {
+      type: 'debugdata',
+      jsLibs: libraryVulns,


from looking at the samplejson here's my understanding:

libraryVulns currently has all js-libs detects and there's an array of vulns attached to each. that array may be empty.

just enumerating all js-libs isn't neccessary cuz we now have the js-libraries audit which does the same.

but the results of this audit reduces 4 diff vulns into one entry, so (aside from this debugdata) we don't expose the 4 vulns individually.

"debugData": { "type": "debugdata", "jsLibs": [ { "name": "jQuery", "npmPkgName": "jquery", "version": "2.1.1", "vulns": [ { "severity": "Medium", "numericSeverity": 2, "library": "jQuery@2.1.1", "url": "https://snyk.io/vuln/SNYK-JS-JQUERY-567880" }, { "severity": "Medium", "numericSeverity": 2, "library": "jQuery@2.1.1", "url": "https://snyk.io/vuln/SNYK-JS-JQUERY-565129" }, { "severity": "Medium", "numericSeverity": 2, "library": "jQuery@2.1.1", "url": "https://snyk.io/vuln/SNYK-JS-JQUERY-174006" }, { "severity": "Medium", "numericSeverity": 2, "library": "jQuery@2.1.1", "url": "https://snyk.io/vuln/npm:jquery:20150627" } ], "highestSeverity": "Medium" }, { "name": "jQuery (Fast path)", "npmPkgName": "jquery", "version": "", "vulns": [] }, { "name": "WordPress", "version": "", "vulns": [] } ],

it seems reasonable that we keep exposing these vulns individually.

but i think we should do libraryVulns.filter(lib => lib.vulns.length)

and instead of jsLibs let's call it vulnerableLibs

beyond that I think this data structure works. (as opposed to not grouping vulns by their lib)

paulirish · 2020-05-15T19:03:17Z

lighthouse-core/audits/accessibility/axe-audit.js

@@ -103,9 +103,6 @@ class AxeAudit extends Audit {

    return {
      score: Number(rule === undefined),
-      extendedInfo: {
-        value: rule,


we don't expose the full axe rule output otherwise.

but i think that's okay

here's a full "rule":

we'd miss out on their description & title and url.
and for each node we'd miss out on its specific impact and failureSummary

i think this is ok.

somewhat related: failureSummary tbh looks useful for our regular output. requires a bit more investigation.

Should I add a note or TODO here?

and for each node we'd miss out on its specific impact and failureSummary

failureSummary becomes each node's explanation in the axe audit tables, e.g.

lighthouse/lighthouse-core/test/results/sample_v2.json

Lines 1965 to 1972 in e2bfa14

"node": {

"type": "node",

"selector": "h2",

"path": "3,HTML,1,BODY,5,DIV,0,H2",

"snippet": "<h2>Do better web tester page</h2>",

"explanation": "Fix any of the following:\n Element has insufficient color contrast of 1.32 (foreground color: #ffc0cb, background color: #eeeeee, font size: 18.0pt, font weight: bold). Expected contrast ratio of 3:1",

"nodeLabel": "Do better web tester page"

}

(after #5402 asked us to put stuff back)

Should I add a TODO or something to remind us to come back to this?

Should I add a TODO or something to remind us to come back to this?

we've been happy with the current output since #5402, so how about let's leave as it is here and re-visit if someone asks for more or we have ideas for future reporting that needs more.

paulirish · 2020-05-15T19:08:45Z

lighthouse-core/audits/user-timings.js

      /** @type {string|undefined} */
      let displayValue;
      if (userTimings.length) {
+        debugData = {
+          type: 'debugdata',
+          userTimings,


looked at this one. i dont think there's any useful data in the raw userTimings obj that we want.

we can drop it.

lighthouse-core/audits/legacy-javascript.js

patrickhulce

From my perspective everything that's currently in extendedInfo can be dropped safely because everything is guaranteed to not be used by the external world, so there should be a good reason for reintroducing.

I know I had like a 3 month period where I forgot about this fact during reviews, so sorry to anyone I asked to put something into extendedInfo 😂

I'm not sure there's much

patrickhulce · 2020-05-15T20:43:12Z

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

+      /** @type {LH.Audit.Details.DebugData} */
+      const debugData = {
+        type: 'debugdata',
+        queryStringCount,


+1 to dropping

patrickhulce · 2020-05-15T20:43:28Z

lighthouse-core/audits/metrics/interactive.js

@@ -74,7 +74,9 @@ class InteractiveMetric extends Audit {
    const timeInMs = metricResult.timing;
    const isDesktop = artifacts.TestedAsMobileDevice === false;
    const options = isDesktop ? context.options.desktop : context.options.mobile;
-    const extendedInfo = {
+    /** @type {LH.Audit.Details.DebugData} */
+    const debugData = {


Nope, let's dump it! ✂️

brendankenny · 2020-05-19T17:58:14Z

From my perspective everything that's currently in extendedInfo can be dropped safely because everything is guaranteed to not be used by the external world, so there should be a good reason for reintroducing.

💯

I don't think we've ever deleted something from debugData, so let's only add stuff there when someone needs it for something. Given that no one's been missing these extendedInfos this whole time, I think we can default to ✂️ ✂️ for all of these

brendankenny · 2020-05-19T18:36:46Z

lighthouse-core/test/results/sample_v2.json

+                }
+              ],
+              "highestSeverity": "Medium"
+            }


Even filtered this still seems mostly redundant. I guess you get to see the different severities per vulnerability, but the majority in our snyk db are medium anyways. Do we want just ['https://snyk.io/vuln/SNYK-JS-JQUERY-567880', 'https://snyk.io/vuln/SNYK-JS-JQUERY-565129', 'https://snyk.io/vuln/SNYK-JS-JQUERY-174006', 'https://snyk.io/vuln/npm:jquery:20150627'] out of this, which captures almost all of the unique additional data? The link we currently provide (https://snyk.io/vuln/npm:jquery?lh=2.1.1) does already have all that for interested parties...

@paulirish Thoughts?

I think there are two cases for use here:

a human or script looking at an individual LHR

most of the time you just want to know what you're using that's vulnerable and that you should upgrade

if you want to dig in, everything you'd want is already at the URL we provide for each library found (that page links off to all the vulnerabilities individually, severities, CVEs, etc)

if a tool really want to show all that detail in their own UI, snyk has both an API and local databases for library@version -> vulnerabilities :)

otoh, if one were to do an HTTP Archive query about vulnerable libraries, I'd imagine the things you'd want out of the LHR are, in decreasing order of likelihood of interest:

the vulnerable version of the library found in a page

the highest vulnerability level found (e.g. select results only from the pages with a High severity vulnerability)

the number of vulnerabilities found for that library

I can't imagine any circumstance in which you'd want the set of all vulnerability levels found for that library (['Medium', 'Medium', 'Medium', 'Medium']), for instance. What would you do with that?

I think this is another case of YAGNI. Regardless of possible scenarios, in practice no one has missed this extra info since extendedInfo went away, so let's add it back if and when someone has a real need for it. That also nicely avoids having to come up with which properties to keep and which to ditch and what shape object should they be put in, etc :)

sounds good. let's drop.

brendankenny · 2020-05-27T19:29:52Z

lighthouse-core/audits/accessibility/axe-audit.js

@@ -103,9 +103,6 @@ class AxeAudit extends Audit {

    return {
      score: Number(rule === undefined),
-      extendedInfo: {
-        value: rule,


Should I add a TODO or something to remind us to come back to this?

we've been happy with the current output since #5402, so how about let's leave as it is here and re-visit if someone asks for more or we have ideas for future reporting that needs more.

brendankenny · 2020-05-27T19:34:10Z

lighthouse-core/test/audits/user-timing-test.js

@@ -41,28 +36,4 @@ describe('Performance: user-timings audit', () => {
      assert.equal(auditResult.details.items[1].duration, undefined);
    });
  });
-
-  it('doesn\'t throw when user_timing events have a colon', () => {


it seems like this was maybe a regression test added at some point. Any reason to delete instead of switching over to checking items? (looks like the same name filter should work)

lighthouse-core/test/audits/user-timing-test.js

brendankenny · 2020-05-27T19:52:59Z

lighthouse-core/test/results/sample_v2.json

+                }
+              ],
+              "highestSeverity": "Medium"
+            }


I think there are two cases for use here:

a human or script looking at an individual LHR

most of the time you just want to know what you're using that's vulnerable and that you should upgrade

if you want to dig in, everything you'd want is already at the URL we provide for each library found (that page links off to all the vulnerabilities individually, severities, CVEs, etc)

if a tool really want to show all that detail in their own UI, snyk has both an API and local databases for library@version -> vulnerabilities :)

otoh, if one were to do an HTTP Archive query about vulnerable libraries, I'd imagine the things you'd want out of the LHR are, in decreasing order of likelihood of interest:

the vulnerable version of the library found in a page

the highest vulnerability level found (e.g. select results only from the pages with a High severity vulnerability)

the number of vulnerabilities found for that library

I can't imagine any circumstance in which you'd want the set of all vulnerability levels found for that library (['Medium', 'Medium', 'Medium', 'Medium']), for instance. What would you do with that?

I think this is another case of YAGNI. Regardless of possible scenarios, in practice no one has missed this extra info since extendedInfo went away, so let's add it back if and when someone has a real need for it. That also nicely avoids having to come up with which properties to keep and which to ditch and what shape object should they be put in, etc :)

paulirish · 2020-06-24T01:38:00Z

lighthouse-core/audits/dobetterweb/no-vulnerable-libraries.js

@@ -226,15 +226,15 @@ class NoVulnerableLibrariesAudit extends Audit {
      {key: 'highestSeverity', itemType: 'text', text: str_(UIStrings.columnSeverity)},
    ];
    const details = Audit.makeTableDetails(headings, vulnerabilityResults, {});
-
+    /** @type {LH.Audit.Details.DebugData} */
+    const debugData = {


as discussed, let's drop.

paulirish · 2020-06-24T01:38:20Z

lighthouse-core/test/results/sample_v2.json

+                }
+              ],
+              "highestSeverity": "Medium"
+            }


sounds good. let's drop.

paulirish

so we'll drop the vuln stuff.

got two conflicts to resolve, but they're both straightforward.

aside from that, we're LGTM

paulirish · 2020-06-24T01:38:54Z

lighthouse-core/test/audits/dobetterweb/no-vulnerable-libraries-test.js

@@ -37,7 +37,7 @@ describe('Avoids front-end JavaScript libraries with known vulnerabilities', ()
    });
    assert.equal(auditResult.score, 0);
    assert.equal(auditResult.details.items.length, 1);
-    assert.equal(auditResult.extendedInfo.jsLibs.length, 3);
+    assert.equal(auditResult.details.debugData.vulnerableLibs.length, 1);


(drop this and L93)

brendankenny · 2020-06-29T21:32:21Z

So long extendedInfo 👋 👋

Added way back in #301 for audit results in the report, though the first use was in a commit predating the Lighthouse repo itself (d2545b0), back in the bigrig times.

paullewis · 2020-06-30T07:17:32Z

@brendankenny Good times! sniff

Beytoven added 3 commits April 16, 2020 19:11

ExtendedInfo Removal pt. 1

2a493e9

Merge branch 'master' into remove-extended-info

193906c

Remove extendedInfo field from audits

c00dab2

Beytoven requested a review from a team as a code owner May 14, 2020 01:09

Beytoven requested review from paulirish and removed request for a team May 14, 2020 01:09

googlebot added the cla: yes label May 14, 2020

Lint fixes

4aeb4dd

vercel bot deployed to Preview May 14, 2020 01:44 View deployment

Update sample json

f97eb3b

vercel bot deployed to Preview May 14, 2020 02:00 View deployment

devtools-bot assigned paulirish May 14, 2020

devtools-bot added the waiting4reviewer label May 14, 2020

paulirish requested changes May 15, 2020

View reviewed changes

patrickhulce reviewed May 15, 2020

View reviewed changes

Remove no longer needed debugdata

93c3ec6

vercel bot deployed to Preview May 19, 2020 03:52 View deployment

brendankenny reviewed May 19, 2020

View reviewed changes

connorjclark requested review from paulirish and brendankenny May 27, 2020 19:16

brendankenny reviewed May 27, 2020

View reviewed changes

Beytoven added 2 commits May 28, 2020 22:32

Merge branch 'master' into remove-extended-info

7d4a2d6

Update tests

daf6900

vercel bot deployed to Preview May 29, 2020 06:16 View deployment

brendankenny mentioned this pull request Jun 19, 2020

core(is-on-https): add mixed-content resolution #10975

Merged

paulirish reviewed Jun 24, 2020

View reviewed changes

paulirish approved these changes Jun 24, 2020

View reviewed changes

paulirish added waiting4committer and removed waiting4reviewer labels Jun 24, 2020

paulirish assigned Beytoven and unassigned paulirish Jun 24, 2020

Beytoven added 3 commits June 24, 2020 15:05

Remove debugData for vulnerable libs audit

28bdf23

Merge branch 'master' into remove-extended-info

fb2cfca

Fix text after merge

00e6a37

vercel bot deployed to Preview June 25, 2020 00:00 View deployment

Merge branch 'master' into remove-extended-info

3b964fa

vercel bot deployed to Preview June 29, 2020 19:04 View deployment

Beytoven merged commit 6159218 into master Jun 29, 2020

Beytoven deleted the remove-extended-info branch June 29, 2020 19:57

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Jul 6, 2020

core: remove uses of deprecated extendedInfo field (GoogleChrome#10779)

d4acde6

brendankenny mentioned this pull request Jul 7, 2020

misc: remove last extendedInfo in LH.Audit.Product #11067

Merged

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Aug 28, 2020

core: remove uses of deprecated extendedInfo field (GoogleChrome#10779)

d6dae68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: remove uses of deprecated extendedInfo field #10779

core: remove uses of deprecated extendedInfo field #10779

Beytoven commented May 14, 2020 •

edited

Loading

paulirish left a comment

paulirish May 15, 2020

patrickhulce May 15, 2020

paulirish May 15, 2020

patrickhulce May 15, 2020

paulirish May 15, 2020

paulirish May 15, 2020

paulirish May 15, 2020

paulirish May 15, 2020

Beytoven May 19, 2020

brendankenny May 19, 2020

brendankenny May 19, 2020

Beytoven May 19, 2020

brendankenny May 27, 2020

paulirish May 15, 2020

patrickhulce left a comment

patrickhulce May 15, 2020

patrickhulce May 15, 2020

brendankenny commented May 19, 2020

brendankenny May 19, 2020

Beytoven May 19, 2020

brendankenny May 27, 2020

paulirish Jun 24, 2020

brendankenny May 27, 2020

brendankenny May 27, 2020

brendankenny May 27, 2020

paulirish Jun 24, 2020

paulirish Jun 24, 2020

paulirish left a comment

paulirish Jun 24, 2020

brendankenny commented Jun 29, 2020

paullewis commented Jun 30, 2020

	"node": {
	"type": "node",
	"selector": "h2",
	"path": "3,HTML,1,BODY,5,DIV,0,H2",
	"snippet": "<h2>Do better web tester page</h2>",
	"explanation": "Fix any of the following:\n Element has insufficient color contrast of 1.32 (foreground color: #ffc0cb, background color: #eeeeee, font size: 18.0pt, font weight: bold). Expected contrast ratio of 3:1",
	"nodeLabel": "Do better web tester page"
	}

core: remove uses of deprecated extendedInfo field #10779

core: remove uses of deprecated extendedInfo field #10779

Conversation

Beytoven commented May 14, 2020 • edited Loading

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

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 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 commented May 19, 2020

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

brendankenny commented Jun 29, 2020

paullewis commented Jun 30, 2020

Beytoven commented May 14, 2020 •

edited

Loading