core(legacy-javascript): use source maps #10564

connorjclark · 2020-04-09T01:53:02Z

If source maps are present, use our knowledge of what node module a polyfill comes from to find signals. Pattern matching takes precedence - the line/column information is more accurate. Otherwise, the first mapping entry for the relevant module is used as the source location.

Because the case of no source maps is important by itself, I introduced a second summary json file in the test.

Diff between the two new summary jsons (This represents the improvement in using maps + pattern matching) (same as what github shows for summary-signals.json in this PR, but this markdown is a bit nicer to look at) :

diff --git a/summary-signals-nomaps.json b/summary-signals.json
index 690bd7b27..022109f5e 100644
--- a/summary-signals-nomaps.json
+++ b/summary-signals.json
@@ -1,51 +1,13 @@
 {
-  "totalSignals": 203,
+  "totalSignals": 251,
   "variantsMissingSignals": [
-    "core-js-2-only-polyfill/es6-array-filter",
-    "core-js-2-only-polyfill/es6-array-map",
-    "core-js-2-only-polyfill/es6-array-reduce",
-    "core-js-2-only-polyfill/es6-array-reduce-right",
-    "core-js-2-only-polyfill/es6-array-some",
-    "core-js-2-only-polyfill/es6-date-to-string",
-    "core-js-2-only-polyfill/es6-function-name",
-    "core-js-2-only-polyfill/es6-object-freeze",
-    "core-js-2-only-polyfill/es6-object-get-own-property-descriptor",
-    "core-js-2-only-polyfill/es6-object-get-own-property-names",
-    "core-js-2-only-polyfill/es6-object-get-prototype-of",
-    "core-js-2-only-polyfill/es6-object-is-extensible",
-    "core-js-2-only-polyfill/es6-object-is-frozen",
-    "core-js-2-only-polyfill/es6-object-is-sealed",
-    "core-js-2-only-polyfill/es6-object-keys",
-    "core-js-2-only-polyfill/es6-object-prevent-extensions",
-    "core-js-2-only-polyfill/es6-object-seal",
-    "core-js-2-only-polyfill/es6-reflect-define-property",
-    "core-js-2-only-polyfill/es6-string-trim",
     "core-js-2-preset-env-esmodules/true",
-    "core-js-3-only-polyfill/es6-array-filter",
-    "core-js-3-only-polyfill/es6-array-map",
-    "core-js-3-only-polyfill/es6-array-reduce",
-    "core-js-3-only-polyfill/es6-array-reduce-right",
-    "core-js-3-only-polyfill/es6-array-some",
-    "core-js-3-only-polyfill/es6-date-to-string",
-    "core-js-3-only-polyfill/es6-function-name",
-    "core-js-3-only-polyfill/es6-object-freeze",
-    "core-js-3-only-polyfill/es6-object-get-own-property-descriptor",
-    "core-js-3-only-polyfill/es6-object-get-own-property-names",
-    "core-js-3-only-polyfill/es6-object-get-prototype-of",
-    "core-js-3-only-polyfill/es6-object-is-extensible",
-    "core-js-3-only-polyfill/es6-object-is-frozen",
-    "core-js-3-only-polyfill/es6-object-is-sealed",
-    "core-js-3-only-polyfill/es6-object-keys",
-    "core-js-3-only-polyfill/es6-object-prevent-extensions",
-    "core-js-3-only-polyfill/es6-object-seal",
-    "core-js-3-only-polyfill/es6-reflect-define-property",
-    "core-js-3-only-polyfill/es6-string-trim",
     "core-js-3-preset-env-esmodules/true"
   ],
   "variants": [
     {
       "name": "core-js-2-only-polyfill/es6-array-filter",
-      "signals": ""
+      "signals": "Array.prototype.filter"
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-find",
@@ -53,7 +15,7 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-find-index",
-      "signals": "Array.prototype.findIndex"
+      "signals": "Array.prototype.findIndex, Array.prototype.find"
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-for-each",
@@ -73,7 +35,7 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-map",
-      "signals": ""
+      "signals": "Array.prototype.map"
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-of",
@@ -81,15 +43,15 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-reduce",
-      "signals": ""
+      "signals": "Array.prototype.reduce"
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-reduce-right",
-      "signals": ""
+      "signals": "Array.prototype.reduce, Array.prototype.reduceRight"
     },
     {
       "name": "core-js-2-only-polyfill/es6-array-some",
-      "signals": ""
+      "signals": "Array.prototype.some"
     },
     {
       "name": "core-js-2-only-polyfill/es6-date-now",
@@ -105,11 +67,11 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-date-to-string",
-      "signals": ""
+      "signals": "Date.prototype.toString"
     },
     {
       "name": "core-js-2-only-polyfill/es6-function-name",
-      "signals": ""
+      "signals": "Function.prototype.name"
     },
     {
       "name": "core-js-2-only-polyfill/es6-map",
@@ -149,43 +111,43 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-freeze",
-      "signals": ""
+      "signals": "Object.freeze"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-get-own-property-descriptor",
-      "signals": ""
+      "signals": "Object.getOwnPropertyDescriptor"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-get-own-property-names",
-      "signals": ""
+      "signals": "Object.getOwnPropertyNames"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-get-prototype-of",
-      "signals": ""
+      "signals": "Object.getPrototypeOf"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-is-extensible",
-      "signals": ""
+      "signals": "Object.isExtensible"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-is-frozen",
-      "signals": ""
+      "signals": "Object.isFrozen"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-is-sealed",
-      "signals": ""
+      "signals": "Object.isSealed"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-keys",
-      "signals": ""
+      "signals": "Object.keys"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-prevent-extensions",
-      "signals": ""
+      "signals": "Object.preventExtensions"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-seal",
-      "signals": ""
+      "signals": "Object.seal"
     },
     {
       "name": "core-js-2-only-polyfill/es6-object-set-prototype-of",
@@ -205,7 +167,7 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-define-property",
-      "signals": ""
+      "signals": "Reflect.defineProperty"
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-delete-property",
@@ -217,11 +179,11 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-get-own-property-descriptor",
-      "signals": "Reflect.getOwnPropertyDescriptor"
+      "signals": "Reflect.getOwnPropertyDescriptor, Reflect.get"
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-get-prototype-of",
-      "signals": "Reflect.getPrototypeOf"
+      "signals": "Reflect.getPrototypeOf, Reflect.get"
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-has",
@@ -245,7 +207,7 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-reflect-set-prototype-of",
-      "signals": "Reflect.setPrototypeOf"
+      "signals": "Reflect.setPrototypeOf, Reflect.set"
     },
     {
       "name": "core-js-2-only-polyfill/es6-set",
@@ -281,7 +243,7 @@
     },
     {
       "name": "core-js-2-only-polyfill/es6-string-trim",
-      "signals": ""
+      "signals": "String.prototype.trim"
     },
     {
       "name": "core-js-2-only-polyfill/es6-typed-array-buffer",
@@ -369,7 +331,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-filter",
-      "signals": ""
+      "signals": "Array.prototype.filter"
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-find",
@@ -377,7 +339,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-find-index",
-      "signals": "Array.prototype.findIndex"
+      "signals": "Array.prototype.findIndex, Array.prototype.find"
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-for-each",
@@ -397,7 +359,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-map",
-      "signals": ""
+      "signals": "Array.prototype.map"
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-of",
@@ -405,15 +367,15 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-reduce",
-      "signals": ""
+      "signals": "Array.prototype.reduce"
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-reduce-right",
-      "signals": ""
+      "signals": "Array.prototype.reduce, Array.prototype.reduceRight"
     },
     {
       "name": "core-js-3-only-polyfill/es6-array-some",
-      "signals": ""
+      "signals": "Array.prototype.some"
     },
     {
       "name": "core-js-3-only-polyfill/es6-date-now",
@@ -429,11 +391,11 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-date-to-string",
-      "signals": ""
+      "signals": "Date.prototype.toString"
     },
     {
       "name": "core-js-3-only-polyfill/es6-function-name",
-      "signals": ""
+      "signals": "Function.prototype.name"
     },
     {
       "name": "core-js-3-only-polyfill/es6-map",
@@ -473,43 +435,43 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-freeze",
-      "signals": ""
+      "signals": "Object.freeze"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-get-own-property-descriptor",
-      "signals": ""
+      "signals": "Object.getOwnPropertyDescriptor"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-get-own-property-names",
-      "signals": ""
+      "signals": "Object.getOwnPropertyNames"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-get-prototype-of",
-      "signals": ""
+      "signals": "Object.getPrototypeOf"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-is-extensible",
-      "signals": ""
+      "signals": "Object.isExtensible"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-is-frozen",
-      "signals": ""
+      "signals": "Object.isFrozen"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-is-sealed",
-      "signals": ""
+      "signals": "Object.isSealed"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-keys",
-      "signals": ""
+      "signals": "Object.keys"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-prevent-extensions",
-      "signals": ""
+      "signals": "Object.preventExtensions"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-seal",
-      "signals": ""
+      "signals": "Object.seal"
     },
     {
       "name": "core-js-3-only-polyfill/es6-object-set-prototype-of",
@@ -529,7 +491,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-define-property",
-      "signals": ""
+      "signals": "Reflect.defineProperty"
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-delete-property",
@@ -541,11 +503,11 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-get-own-property-descriptor",
-      "signals": "Reflect.getOwnPropertyDescriptor"
+      "signals": "Reflect.getOwnPropertyDescriptor, Reflect.get"
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-get-prototype-of",
-      "signals": "Reflect.getPrototypeOf"
+      "signals": "Reflect.getPrototypeOf, Reflect.get"
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-has",
@@ -569,7 +531,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-reflect-set-prototype-of",
-      "signals": "Reflect.setPrototypeOf"
+      "signals": "Reflect.setPrototypeOf, Reflect.set"
     },
     {
       "name": "core-js-3-only-polyfill/es6-set",
@@ -605,7 +567,7 @@
     },
     {
       "name": "core-js-3-only-polyfill/es6-string-trim",
-      "signals": ""
+      "signals": "String.prototype.trim"
     },
     {
       "name": "core-js-3-only-polyfill/es6-typed-array-buffer",

…ps in test variants

patrickhulce

nice work! another carrot for the sourcemapping folks :)

lighthouse-core/audits/legacy-javascript.js

lighthouse-core/scripts/legacy-javascript/run.js

patrickhulce · 2020-04-09T15:29:12Z

lighthouse-core/test/audits/legacy-javascript-test.js

@@ -28,6 +28,7 @@ const createArtifacts = (scripts) => {
      };
      return acc;
    }, {}),
+    SourceMaps: [],


could we a unit test or two in here to exercise the deduplication piece and any other flow that isn't currently covered by the bash script?

Co-Authored-By: Patrick Hulce <[email protected]>

…nto legacy-js-maps

connorjclark · 2020-04-13T22:57:02Z

@patrickhulce bump :)

patrickhulce

LGTM

travis says legacy-javascript smoketest failed to produce a report though so I'm a little concerned what I'm missing :/

connorjclark · 2020-04-13T23:41:22Z

I'm not bothering to look until #10566 lands, maybe moot.

connorjclark · 2020-04-22T23:48:51Z

lighthouse-core/scripts/legacy-javascript/run.js

@@ -191,7 +76,8 @@ async function createVariant(options) {
    fs.writeFileSync(`${dir}/.babelrc`, JSON.stringify(babelrc || {}, null, 2));
    // Not used in this script, but useful for running Lighthouse manually.
    // Just need to start a web server first.
-    fs.writeFileSync(`${dir}/index.html`, `<title>${name}</title><script src=main.bundle.min.js>`);


lack of closing script element = bad sad times w/ no debuggability in CDT. chrome still requests the script, still shows in Network as a script, yet never loads.

connorjclark · 2020-04-22T23:54:15Z

lighthouse-core/scripts/legacy-javascript/summary-signals.json

    },
    {
      "name": "core-js-2-only-polyfill/es6-array-find",
      "signals": "Array.prototype.find"
    },
    {
      "name": "core-js-2-only-polyfill/es6-array-find-index",
-      "signals": "Array.prototype.findIndex"
+      "signals": "Array.prototype.findIndex, Array.prototype.find"


Array.prototype.find is a false positive

connorjclark · 2020-04-22T23:55:54Z

lighthouse-core/scripts/legacy-javascript/summary-signals.json

@@ -137,45 +29,33 @@
      "name": "core-js-2-only-polyfill/es6-array-from",
      "signals": "Array.from"
    },
-    {
-      "name": "core-js-2-only-polyfill/es6-array-index-of",
-      "signals": ""


this were empty because legacy javascript never checked for these polyfills, because I omitted them intentionally. The duplication of the polys in the run.js script had some extra stuff. fixed now they it uses the same data source.

connorjclark · 2020-04-22T23:57:16Z

lighthouse-core/scripts/legacy-javascript/summary-signals.json

    {
      "name": "core-js-3-only-polyfill/es6-array-last-index-of",
      "signals": "Array.prototype.lastIndexOf"
    },
    {
      "name": "core-js-3-only-polyfill/es6-array-map",
-      "signals": ""
+      "signals": "Array.prototype.map"


example of the pattern matching not yielding results, but using source maps filled a gap.

connorjclark · 2020-04-22T23:58:29Z

lighthouse-core/scripts/legacy-javascript/summary-signals.json

    },
    {
      "name": "core-js-3-only-polyfill/es6-array-reduce-right",
-      "signals": ""
+      "signals": "Array.prototype.reduce, Array.prototype.reduceRight"


another false positive. I think the map sources substring check is too loose.

connorjclark · 2020-04-23T00:20:58Z

lighthouse-core/audits/legacy-javascript.js

+          // Skip if the pattern matching found a match for this polyfill.
+          if (matches.some(m => m.name === name)) continue;
+
+          const source = bundle.rawMap.sources.find(source => source.includes(module));


I was avoiding endsWith here because I know that webpack sometimes adds ? to the end of things ... I don't know if that's actually true from a sane configuration and maybe that's not an issue.

@patrickhulce think we can just uses endsWith here`? should we try to strip anything after a question mark ?

a86f324 fixed many false positives

does it need to be endsWith or something more like

new RegExp(`${module}($|\/|\\?)`

perhaps? these are the file names right?

yeah this is the entire file path, resolvable in node. I don't think those slashes are needed. module is the name like es.array.fill and the node module is named ...es.array.fill.js.

I'm just unclear if sometimes there is a ? in webpack maps and if I should account for that.

connorjclark · 2020-04-23T00:33:08Z

lighthouse-core/scripts/legacy-javascript/summary-sizes.txt

@@ -1,238 +1,176 @@
 core-js-2-only-polyfill
-  47738        es6-typed-uint8-clamped-array/main.bundle.min.js


source map comments made everything bigger

updates to look at

patrickhulce · 2020-04-23T20:46:02Z

I'm a little unclear what the status is here @connorjclark. Marked as waiting on me so presumably the blockers were cleared, but the linked issue was an issue not a PR (but I guess its linked PR landed).

Are the current build failures expected? The only substantial change I see since my last review is the endsWith change I have an open question on :)

connorjclark · 2020-04-23T23:54:27Z

note: runner unit test is flaky in GH Run xvfb-run --auto-servernum yarn unit https://github.com/GoogleChrome/lighthouse/pull/10564/checks?check_run_id=613725231

connorjclark · 2020-04-23T23:58:33Z

I'm a little unclear what the status is here @connorjclark. Marked as waiting on me so presumably the blockers were cleared, but the linked issue was an issue not a PR (but I guess its linked PR landed).

Are the current build failures expected? The only substantial change I see since my last review is the endsWith change I have an open question on :)

Sorry, I jumped the gun there. It should be ready to land now. I commented a lot to explain why some changes happened.

patrickhulce · 2020-04-24T00:14:23Z

note: runner unit test is flaky in GH Run xvfb-run --auto-servernum yarn unit

maybe we should increase the timeout?

patrickhulce

LGTM

connorjclark added 2 commits April 8, 2020 15:54

core(legacy-javascript): add structured polyfill data; emit source ma…

7c554ac

…ps in test variants

core(legacy-javascript): use source maps

457a953

connorjclark requested a review from a team as a code owner April 9, 2020 01:53

connorjclark requested review from patrickhulce and removed request for a team April 9, 2020 01:53

googlebot added the cla: yes label Apr 9, 2020

devtools-bot assigned patrickhulce Apr 9, 2020

devtools-bot added the waiting4reviewer label Apr 9, 2020

tsc

1759f20

vercel bot deployed to Preview April 9, 2020 02:14 View deployment

patrickhulce suggested changes Apr 9, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Apr 9, 2020

connorjclark mentioned this pull request Apr 9, 2020

test-legacy-javascript improvements #10566

Closed

3 tasks

Apply suggestions from code review

3fcaabd

Co-Authored-By: Patrick Hulce <[email protected]>

vercel bot deployed to Preview April 9, 2020 23:01 View deployment

connorjclark added 2 commits April 9, 2020 16:04

Merge remote-tracking branch 'origin/master' into legacy-js-maps

52ca359

Merge branch 'legacy-js-maps' of github.com:GoogleChrome/lighthouse i…

51f5b9c

…nto legacy-js-maps

vercel bot deployed to Preview April 9, 2020 23:12 View deployment

test

1ab77f7

vercel bot deployed to Preview April 10, 2020 21:02 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Apr 10, 2020

connorjclark mentioned this pull request Apr 13, 2020

tests(legacy-javascript): cache variants, check in lockfile, … #10568

Merged

patrickhulce previously approved these changes Apr 13, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Apr 13, 2020

Merge remote-tracking branch 'origin/master' into legacy-js-maps

5a5ff74

vercel bot deployed to Preview April 22, 2020 23:44 View deployment

connorjclark commented Apr 22, 2020

View reviewed changes

connorjclark commented Apr 23, 2020

View reviewed changes

use endsWith, which removes many false positives

69d0b6b

vercel bot deployed to Preview April 23, 2020 00:30 View deployment

connorjclark commented Apr 23, 2020

View reviewed changes

connorjclark added waiting4reviewer and removed waiting4committer labels Apr 23, 2020

fix

a86f324

vercel bot deployed to Preview April 23, 2020 00:40 View deployment

patrickhulce added waiting4committer and removed waiting4reviewer labels Apr 23, 2020

fix test

e4f3bb9

vercel bot deployed to Preview April 23, 2020 23:09 View deployment

smoke

8ac8355

vercel bot deployed to Preview April 23, 2020 23:43 View deployment

patrickhulce approved these changes Apr 24, 2020

View reviewed changes

connorjclark merged commit 80e24f1 into master Apr 27, 2020

connorjclark deleted the legacy-js-maps branch April 27, 2020 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(legacy-javascript): use source maps #10564

core(legacy-javascript): use source maps #10564

connorjclark commented Apr 9, 2020 •

edited

Loading

patrickhulce left a comment

patrickhulce Apr 9, 2020

connorjclark Apr 10, 2020

connorjclark commented Apr 13, 2020

patrickhulce left a comment

connorjclark commented Apr 13, 2020

connorjclark Apr 22, 2020

connorjclark Apr 22, 2020

connorjclark Apr 22, 2020

connorjclark Apr 22, 2020

connorjclark Apr 22, 2020

connorjclark Apr 23, 2020 •

edited

Loading

connorjclark Apr 23, 2020 •

edited

Loading

patrickhulce Apr 23, 2020

connorjclark Apr 23, 2020

connorjclark Apr 23, 2020

patrickhulce commented Apr 23, 2020

connorjclark commented Apr 23, 2020

connorjclark commented Apr 23, 2020

patrickhulce commented Apr 24, 2020

patrickhulce left a comment

		@@ -1,238 +1,176 @@
		core-js-2-only-polyfill
		47738 es6-typed-uint8-clamped-array/main.bundle.min.js

core(legacy-javascript): use source maps #10564

core(legacy-javascript): use source maps #10564

Conversation

connorjclark commented Apr 9, 2020 • edited Loading

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

connorjclark commented Apr 13, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented Apr 13, 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

connorjclark Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

connorjclark Apr 23, 2020 • 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 commented Apr 23, 2020

connorjclark commented Apr 23, 2020

connorjclark commented Apr 23, 2020

patrickhulce commented Apr 24, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented Apr 9, 2020 •

edited

Loading

connorjclark Apr 23, 2020 •

edited

Loading

connorjclark Apr 23, 2020 •

edited

Loading