new_audit(legacy-javascript): detect unnecessary polyfills and transforms by connorjclark · Pull Request #6730 · GoogleChrome/lighthouse

connorjclark · 2018-12-05T19:22:37Z

EDIT: jump to here for updates.

I imagine this will go through a few iterations.

@paulirish had the idea to surface what polyfills are in use, and suggest removing them. It sounded like fun so I took pass at it.

First, let's talk detecting polyfills. I thought about a few ways to do it, but just two were viable. We could parse out an AST to extract where native functions were being redefined. But, that's likely to be slow, and would add quite a large dependency. Instead, I opted for somewhat-simple regex parsing. The grammar for setting a property (either = or Object.defineProperty) is fairly simple to grok for, and ought to be much faster. So I went with regex.

Second, let's talk what suggestions to make. I'm not sure we can concretely suggest removing any polyfills. Developers often must support an audience that uses old browsers, so suggesting removing polyfills for features present in the newest browsers would not be found agreeable. Even if a feature is found in browsers going back to IE8, it can still be the wrong call for some sites. So I suggest not attaching any score to this audit, and keeping the language non-prescriptive. Instead, we can just surface what polyfills exist and suggest that they consider researching what they actually need.

Some things worth talking about:

Detecting modules that re-implement native JS features, but don't actually polyfill anything. For example, bundling lodash/startsWith for a web target, and using that instead of String.prototype.startsWith. Bundlers tend to strip module names in production, so such detection likely requires source maps. Otherwise, we're left with analyzing what these modules look like minified. This could certainly be explored further in a separate feature.
If a polyfill appears multiple times from multiple scripts, I display a counter in the report. I imagine this would only be actionable if the polyfills are coming from the same origin - if two third party scripts are polyfilling the same thing, there's nothing really to be done. Should we suggest action only if multiple polyfills are coming from the same origin? If so, what's a good way to present that suggestion?
The location in the script that the polyfill occurs is provided in the report. Usually, this script is minified, so really it's just an index into an unreadable buffer... Is this useful at all? Should this be removed and only added back with source map support?
I grabbed a list of features that can be polyfilled from here. Certainly not exhaustive. From where should we source this list?

Related: #3862

patrickhulce · 2018-12-05T19:43:16Z

DZL, do a barrel roll!

patrickhulce · 2018-12-05T19:59:46Z

DZL is done! Go check it out http://lh-dzl-6730.surge.sh

connorjclark · 2018-12-05T20:04:15Z

CNN

patrickhulce

First off, I 😍 this!!

I'll say that I think we should take a slightly stronger position on polyfills. Even if users legitimately need polyfills for old IE, there's no reason they need to serve them to their Chrome users (which we know the version of the page we're looking at is the Chrome-one). I'd be happy marking this as a failure with language that points them on how to do the philwalton-style conditional serving.

Detecting modules that reimplement native JS features, but don't actually pollyfil anything. For example, bundling lodash/startsWith for a web target, and using that instead of String.prototype.startsWith. Bundlers tend to strip module names in production, so such detection likely requires source maps. Otherwise, we're left with analyzing what these modules look like minified. This could certainly be explored further in a separate feature.

I think this is a perfect candidate for the bundle analysis initiative, agreed it should wait for another time 👍

If a polyfill appears multiple times from multiple scripts, I display a counter in the report. I imagine this would only be actionable if the polyfills are coming from the same origin - if two third party scripts are pollyfilling the same thing, there's nothing really to be done. Should we suggest action only if multiple pollyfills are coming from the same origin? If so, what's a good way to present that suggestion?

I think polyfills from third party origins is already going to be a thorny/unsolvable issue for the user, so I don't think it terribly matters. Removing any unnecessary polyfill is a win, so I think showing each of them separately like you are is 👍

Is this useful at all? Should this be removed and only added back with source map support?

I think including it is fine, agreed it's not very useful, but it's better than nothing.

patrickhulce · 2018-12-05T19:56:41Z

+      // TODO this would only work if the bundle has not been stripped of module names (and replaced with an index into the bundled modules),
+      // which is unlikely in a production environment. Instead, look at source map?
+      // TODO how to detect if all of lodash is bundled (a waste)?
+      // TODO maybe just punt entirely for now?


patrickhulce · 2018-12-05T19:56:54Z

+      let subpattern = '';
+
+      // String.prototype.startsWith =
+      subpattern += `${object || ''}.${property}\\s*=`;


is that meant to be an optional dot literal? \\.?

patrickhulce · 2018-12-05T19:58:52Z

+  static async audit(artifacts) {
+    // TODO
+    // how do we determine which polys are not necessary?
+    // ex: only reason to polyfill Array.prototype.filter is if IE <9,


Our general position on this in the past has been that we know we're loading the page from Chrome, even if you need them for IE, you shouldn't be serving them to your Chrome users.

That makes a lot of sense, I did not consider that. Very cool that many ES2015+ features can be feature-detected with <script> modules.

patrickhulce · 2018-12-05T19:59:08Z

+        const url = record.url;
        if (content) {
-          scriptContentMap[record.requestId] = content;
+          scriptContentMap[record.requestId] = {


it was done this way originally because everything you want about the record (like URL) can be matched up by the requestId. url is a super common need, so I'm not terribly opposed to having it here, but I wouldn't want that list to grow to other things that can be plucked from the network record.

Would I necessarily need to create a gather for this then? That's what I wanted to avoid.

Nope, shouldn't need any gatherer changes at all, just use Scripts artifact as-is and get the URL from const url = networkRecords.find(record => record.requestId === requestId).url

will have to compute network records from devtools log, but not the end of the world

Sweet - I reverted those changes.

patrickhulce · 2018-12-05T20:05:29Z

+    let result;
+    let row = 0;
+    let rowBeginsAtIndex = 0;
+    while ((result = re.exec(code)) !== null) {


I'm gonna at least need a comment here :)

the ...matches below made me think the results were already somehow all inside one regex match, but the .exec suggests we need to iterate through them all

the ..matches is just the subgroups of each match?

yeah, and only one subgroup is ever defined in each matches. I agree this is arcane bordering on magic. I'll comment accordingly.

patrickhulce · 2018-12-05T20:48:16Z

DZL is done! Go check it out http://lh-dzl-6730.surge.sh

patrickhulce · 2018-12-05T21:45:14Z

DZL is done! Go check it out http://lh-dzl-6730.surge.sh

patrickhulce

not sure where this sits on the priority chain if it's still on your plate, but I'm definitely excited :)

patrickhulce · 2018-12-11T16:02:10Z

+const i18n = require('../lib/i18n/i18n.js');
+
+const UIStrings = {
+  /** Imperative title of a Lighthouse audit that tells the user about all JavaScript polyfills loaded on the page. This is displayed in a list of audit titles that Lighthouse generates. */


this one's not imperative :)

patrickhulce · 2018-12-11T16:04:39Z

+  title: 'Polyfills',
+  /** TODO: write this */
+  // eslint-disable-next-line max-len
+  description: 'Polyfills enable older browsers to use new JavaScript language features. However, they aren\'t always necessary. Research what browsers you must support and consider removing polyfils for features that are well supported by them.',


Maybe our advice here is more like Consider conditionally serving polyfills based on feature availability.?

patrickhulce · 2018-12-11T16:08:20Z

+    //     only reason to polyfill String.prototype.startsWith is if IE, ...
+    // Is there a standard way to declare "we support these browsers"? A meta tag?
+    /** @type {Poly[]} */
+    const polys = [


maybe we could pull some of this code out of the main audit function and break it down a bit?

patrickhulce · 2018-12-11T16:08:55Z

+      {key: 'url', itemType: 'url', text: 'URL'},
+      {key: 'description', itemType: 'code', text: 'Description'},
+      {key: 'location', itemType: 'code', text: 'Location'},
+      // TODO include estimate for size based on https://www.npmjs.com/package/mdn-polyfills#supported-polyfills ?


this is interesting, we could then make it an opportunity instead of a diagnostic too!

I think it might be hilariously small of an impact, though. The average in that table is 300 bytes. Say there's a dozen - about 4KB. is that significant?

We'd probably have to get a little more aggressive then. Some of the babel polyfills are no joke, maybe async/await IIRC, but the impact was more like ~80KB for some bundles.

Yeah in phil's article it was ~95K minified plus some extra parse/eval time savings

https://philipwalton.com/articles/deploying-es2015-code-in-production-today/

But do we know how much of that is generator cruft (async -> es5 code), string templates (aka language level polyfills), vs. web api polyfills?

so far I'm just considering web api polyfills, which is much more easily identifiable than language polyfills. It'd be very powerful to capture those too though. any ideas?

Yeah these are great points!

I wasn't trying to suggest we do it right away these other categories are what I was trying to get at with be more aggressive :) I'm afraid I don't really have any obvious initial ideas we can pursue. Maybe we'll have to take a look at some babelized bundles and look for distinctive markers of cruft, but we can cross that bridge another day for sure 👍

patrickhulce · 2018-12-11T16:09:39Z

        {id: 'errors-in-console', weight: 1},
        {id: 'image-aspect-ratio', weight: 1},
+        // Manual audits
+        {id: 'polyfills', weight: 0},


IMO this belongs in performance as a diagnostic and can be upgraded to an opportunity once we have the size data :)

patrickhulce · 2018-12-11T20:57:19Z

DZL is done! Go check it out http://lh-dzl-6730.surge.sh

patrickhulce

I'd personally love to see this land as a diagnostic in performance :)

From my perspective I think we just need some tests and this is good to go in a 4.1

patrickhulce · 2019-01-16T02:16:21Z

+          url,
+          description: `${polyStatement}${countText}`,
+          // TODO use sourcemaps
+          location: `r: ${row}, c: ${col}`,


we can use line instead of row everywhere, right? or is there a subtle difference I'm missing

patrickhulce · 2019-01-16T02:26:17Z

+   * @param {string} code
+   * @return {PolyIssue[]}
+   */
+  static detectPolys(polys, code) {


nit: but polyfills is only 4 more characters and would make me feel warm and fuzzy looking at all the code :)

devtools-bot · 2019-01-16T02:59:36Z

DZL is done! Go check it out http://lh-dzl-6730.surge.sh

patrickhulce · 2019-01-16T15:03:09Z

Side-note I recently came across a URL in #7027 that had this

Not sure how that's possible, could be related to #7005 but all aboard to polyfill removal train!

connorjclark · 2019-01-16T17:45:49Z

Side-note I recently came across a URL in #7027 that had this

wow that's quite the polyfill file.

Not sure how that's possible, could be related to #7005 but all aboard to polyfill removal train!

could it be another mismatch between expectation & reality re: attribution? similar to #7005

patrickhulce · 2019-01-16T18:04:42Z

could it be another mismatch between expectation & reality re: attribution?

yeah it seems like it's getting credit for all XHR callbacks, probably polyfilling fetch or something

googlebot · 2019-07-24T01:25:22Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-24T01:26:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

patrickhulce

great stuff! mostly comment comments from me, feel free to tweak or disagree :)

patrickhulce · 2019-07-23T16:57:14Z

+              // 42 are from babel-polyfill
+              // 1 from fixtures/legacy-javascript.js
+              // 1 inline from fixtures/legacy-javascript.html
+              length: 44,


man, expected: 44, received: 43 is going to be a fun one to debug huh :)

is it feasible to enumerate somehow?

patrickhulce · 2019-07-24T02:30:03Z

+    let lineBeginsAtIndex = 0;
+    // Each pattern maps to one subgroup in the generated regex. For each iteration of RegExp.exec,
+    // only one subgroup will be defined. Exec until no more matches.
+    this.re.lastIndex = 0;


is this resetting for the loop if .match is called more than once? I was a bit confused by the comment immediately above it, could move a line out of the way perhaps? :)

patrickhulce · 2019-07-24T02:38:47Z

+        tableRows.push({
+          url,
+          description,
+          location: `Ln: ${line}, Col: ${column}`,


source-location candidate? :)

that's the dream

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

codecov · 2019-07-24T04:01:29Z

Codecov Report

Merging #6730 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6730      +/-   ##
==========================================
+ Coverage   91.67%   91.73%   +0.05%     
==========================================
  Files         297      298       +1     
  Lines       10192    10296     +104     
==========================================
+ Hits         9344     9445     +101     
- Misses        848      851       +3

Flag	Coverage Δ
#smoke	`85.36% <98.66%> (+0.9%)`	⬆️
#unit	`90.1% <100%> (+0.09%)`	⬆️

Impacted Files	Coverage Δ
lighthouse-core/config/default-config.js	`87.5% <ø> (ø)`	⬆️
lighthouse-core/audits/legacy-javascript.js	`100% <100%> (ø)`
lighthouse-core/audits/user-timings.js	`96% <0%> (-4%)`	⬇️
lighthouse-core/gather/driver.js	`93.01% <0%> (-0.21%)`	⬇️
lighthouse-core/lib/lh-error.js	`95.74% <0%> (-0.09%)`	⬇️
lighthouse-core/audits/seo/font-size.js	`97.95% <0%> (-0.03%)`	⬇️
.../computed/metrics/cumulative-long-queuing-delay.js
...re/audits/metrics/cumulative-long-queuing-delay.js
...d/metrics/lantern-cumulative-long-queuing-delay.js
...hthouse-core/audits/metrics/total-blocking-time.js	`100% <0%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ec9ff...e2202ee. Read the comment docs.

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

googlebot · 2019-08-12T23:06:48Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

connorjclark · 2019-09-20T02:55:04Z

Because GitHub's way of eliding comments is so awful, here are the last significant comments I made summarizing things: #6730 (comment) #6730 (comment)

And the spreadsheet listing all the babel plugins that should not be present in a module/nomodule setup. And how detectable the transforms are + mapping the babel polyfill module name with what JS function it polyfills: https://docs.google.com/spreadsheets/d/1z28Au8wo8-c2UsM2lDVEOJcI3jOkb2c951xEBqzBKCc/edit?usp=sharing

connorjclark · 2020-02-05T22:47:20Z

Too many comments here, so I am closing this PR. Will reopen after making the updates discussed offline.

connorjclark added 3 commits December 4, 2018 14:19

core: extraneous-polys audit

a001ff6

rework

6eb14ef

mark manual

61cc1d8

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners December 5, 2018 19:22

patrickhulce reviewed Dec 5, 2018

View reviewed changes

pr

aafed57

get url from network records

e157f1a

connorjclark mentioned this pull request Dec 5, 2018

DZL Proposal and Variance Measurement #6152

Closed

fix tests

97c6468

connorjclark requested a review from exterkamp as a code owner December 6, 2018 17:36

patrickhulce reviewed Dec 11, 2018

View reviewed changes

connorjclark added 3 commits December 11, 2018 11:25

refactor audit

797b743

Merge remote-tracking branch 'origin/master' into ext-polys

34874d3

update i18n json

6f98bb6

patrickhulce reviewed Jan 16, 2019

View reviewed changes

connorjclark added 2 commits January 15, 2019 18:45

Merge remote-tracking branch 'origin/master' into ext-polys

e96e252

rename

3bd6daa

make test file

987173b

connorjclark added the cla: yes label Jul 24, 2019

bundle size

9a252ce

vercel Bot deployed to staging July 24, 2019 01:25 View deployment

googlebot added cla: no and removed cla: yes labels Jul 24, 2019

patrickhulce reviewed Jul 24, 2019

View reviewed changes

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

e237569

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel Bot temporarily deployed to staging July 24, 2019 04:01 Inactive

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

e2202ee

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel Bot deployed to staging July 24, 2019 04:01 View deployment

pr

d43d3e2

vercel Bot deployed to staging July 24, 2019 04:18 View deployment

brendankenny removed the waiting4review label Aug 7, 2019

enumerate

baba158

vercel Bot deployed to staging August 12, 2019 23:06 View deployment

googlebot added cla: yes and removed cla: no labels Aug 12, 2019

connorjclark added 5 commits August 12, 2019 16:08

Merge remote-tracking branch 'origin/master' into ext-polys

c3d0adb

comments

3228f57

Merge remote-tracking branch 'origin/master' into ext-polys

01917a0

sample update

6255c81

Merge remote-tracking branch 'origin/master' into ext-polys

aa64506

Merge remote-tracking branch 'origin/master' into ext-polys

31f4954

connorjclark mentioned this pull request Feb 6, 2020

core(legacy-javascript): new audit #10303

Merged

Conversation

connorjclark commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce commented Dec 5, 2018

Uh oh!

patrickhulce commented Dec 5, 2018

Uh oh!

connorjclark commented Dec 5, 2018

Uh oh!

patrickhulce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Dec 5, 2018

Uh oh!

patrickhulce commented Dec 5, 2018

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Dec 11, 2018

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devtools-bot commented Jan 16, 2019

Uh oh!

patrickhulce commented Jan 16, 2019

Uh oh!

connorjclark commented Jan 16, 2019

Uh oh!

connorjclark commented Dec 5, 2018 •

edited

Loading

patrickhulce left a comment •

edited

Loading

codecov Bot commented Jul 24, 2019 •

edited

Loading