core(legacy-javascript): reduce polyfills, fix core-js import in test #10937

connorjclark · 2020-06-10T01:45:29Z

The PR addresses two things:

require('core-js') is vital for preset-env to import any polyfills. The esmodules variants were missing this, so the bundles only differed minimally (transforms). After adding this, I noticed that Object.create was included for core-js@2 + esmodule: true, so I removed that polyfill.
At this point, the core-js@3 esmodules: true variant had many signals–the expectation is that there be none. I realized that many polyfills should not be used as discriminators for identifying an module/nomodule build. See preset-env core-js 3 includes extra polyfills babel/babel#11700 (comment) . This resulted in removing ~25 polyfills (see below).

Int8Array, Uint8ClampedArray, ArrayBuffer, DataView, Map, WeakMap, Promise, Object.getOwnPropertyDescriptor, Array.prototype.lastIndexOf, Number.parseFloat, Object.assign, Reflect.set, Set, String.prototype.endsWith, String.prototype.includes, String.prototype.padEnd, String.prototype.padStart, String.prototype.startsWith, String.prototype.trim, Float32Array, Float64Array, Int16Array, Int32Array, Uint16Array, Uint32Array, Uint8Array

Related: #10852

connorjclark · 2020-06-10T01:46:28Z

lighthouse-cli/test/smokehouse/test-definitions/legacy-javascript/expectations.js

+                    {signal: 'String.prototype.codePointAt'},
+                    {signal: 'String.fromCodePoint'},
+                    {signal: 'String.raw'},
+                    {signal: 'String.prototype.repeat'},


This should have been a clue something was wrong. the legacy-javascript.js fixture here is the core-js@3 esmodules: false variant, which should have dozens of variants.

connorjclark · 2020-06-10T01:48:18Z

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

-    }, /** @type {string[]} */ ([])).join(', ');
-    variants.push({name: dir, signals});
+
+    const signals = [];


#10867 should have failed this test in CI .... fixing here

oh this is a thorny problem. we don't know when to run these tests because something they depend on changed :/

I don't have any great ideas to fix this though other than catching it the next time. you?

but test-legacy-javascript.sh checks for any file with legacy-javascript in the path. It should have triggered on a change to the audit.

well then that's even more alarming :)

my point is still a problem I don't know a good way to deal with, but perhaps less urgent than outright broken testing 😆

it did fail but didn't exit with code 1

https://travis-ci.org/github/GoogleChrome/lighthouse/builds/696156441#L1224

need to add

main().catch(err => { console.error(err); process.exit(1); })

connorjclark · 2020-06-10T01:49:27Z

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

-  1352         true/main.bundle.min.js
+  188400       false/main.bundle.min.js
+   64217       true/main.bundle.min.js
+   63536       true-and-bugfixes/main.bundle.min.js


The small difference from bugfixes: true is because it only reduces the number of transforms applied. And the main.js file used is super small.

oh, and adding bugfixes variant is orthogonal to the rest of this PR, I just wanted to add this.

connorjclark · 2020-06-10T01:50:42Z

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

@@ -144,11 +144,12 @@ describe('LegacyJavaScript audit', () => {
      'String.prototype[\'repeat\'] = function() {}',
      'Object.defineProperty(String.prototype, "repeat", function() {})',
      'Object.defineProperty(String.prototype, \'repeat\', function() {})',
-      'Object.defineProperty(window, \'WeakMap\', function() {})',
+      // Currently are no polyfills that declare a class. May be in the future.


I'm hopeful that we can somehow get preset-modules to workaround the bugs that require Promises to be polyfilled. keeping this around + the code in legacy-javascript that detects this.

patrickhulce

I'm struggling to interpret the output in babel/babel#11700 (comment) a bit. Could you help me out?

Does each row of the polyfill say which target requires that the polyfill be used?

For example for the typed arrays it looks like safari 10.1 forces it, even though Uint8Array was in Safari 5. I'm guessing there is some bug that makes it not behave exactly the same way?

I wish we had a better way to track each of these bugs, what they are/why you need them and/or an even more modern delivery discriminant than esmodules. Some of these are really massive polyfills just for what I'm guessing is like one obscure bug fix of a 3 year old versioned browser.

patrickhulce · 2020-06-10T03:33:42Z

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

-    }, /** @type {string[]} */ ([])).join(', ');
-    variants.push({name: dir, signals});
+
+    const signals = [];


oh this is a thorny problem. we don't know when to run these tests because something they depend on changed :/

I don't have any great ideas to fix this though other than catching it the next time. you?

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

patrickhulce · 2020-06-10T03:35:14Z

lighthouse-core/audits/legacy-javascript.js

-      ['String.prototype.startsWith', 'es6.string.starts-with'],
-      ['String.prototype.trim', 'es6.string.trim'],
-      // These break the coreJs2/coreJs3 naming pattern so are set explicitly.
-      {name: 'ArrayBuffer', coreJs2Module: 'es6.typed.array-buffer', coreJs3Module: 'es.array-buffer.constructor'},


aren't these all the biggest ones? and they're really all included in every esmodules: true bundle? :(

patrickhulce · 2020-06-10T03:35:21Z

lighthouse-core/audits/legacy-javascript.js

      ['Number.parseInt', 'es6.number.parse-int'],
-      ['Object.assign', 'es6.object.assign'],


I'm still incredulous that modern browsers need Object.assign polyfilled... 😢

patrickhulce · 2020-06-10T03:35:29Z

lighthouse-core/audits/legacy-javascript.js

@@ -205,7 +200,6 @@ class LegacyJavascript extends Audit {
      ['Object.preventExtensions', 'es6.object.prevent-extensions'],
      ['Object.seal', 'es6.object.seal'],
      ['Object.setPrototypeOf', 'es6.object.set-prototype-of'],
-      ['Promise', 'es6.promise'],


and promises!?

connorjclark · 2020-06-10T06:04:55Z

I'm struggling to interpret the output in babel/babel#11700 (comment) a bit. Could you help me out?

Does each row of the polyfill say which target requires that the polyfill be used?

Yes. esmodules: true sets the targets to

{
  "android": "61",
  "chrome": "61",
  "edge": "16",
  "firefox": "60",
  "ios": "10.3",
  "opera": "48",
  "safari": "10.1",
  "samsung": "8.2"
}

and each polyfill or transform that matches one of these targets is printed out.

For example for the typed arrays it looks like safari 10.1 forces it, even though Uint8Array was in Safari 5. I'm guessing there is some bug that makes it not behave exactly the same way?

Honestly, I can't figure this out.

core-js@3 uses this data to determine what to pull in: https://github.com/zloirock/core-js/blob/master/packages/core-js-compat/src/data.js#L1037

It seems that because safari is not listed, that it would get polyfilled for all versions of safari (ios too).

If this is WAI I must assume there is some bug in all versions Safari that makes typed arrays inconsistent with other browsers. typed-array was added in core-js@2, can't find much discussion other than this: zloirock/core-js#90

@developit said that all methods that accept a regex parameter must be polyfilled in older browsers. I don't see a similar reason to polyfill the typed arrays.

EDIT: seems it's only b/c of a bug in toString: zloirock/core-js#300 (comment) .

A really nice thing about the caniuse data is that is gives reasons/comments in the data for such things. core-js-compat has none of that, so it's hard for us to work out if this is intentional or not.

I wish we had a better way to track each of these bugs, what they are/why you need them and/or an even more modern delivery discriminant than esmodules

that's the dream ...

@paulirish and I want to explore extending preset-modules to reduce the polyfill footprint. AFAIK it currently only reduces the number of transforms necessary. seems that typed-array may be plausible if it's really just b/c of toString.

Promise is included for a few reasons...

es.promise { "android":"61", "chrome":"61", "edge":"16", "firefox":"60", "ios":"10.3", "opera":"48", "safari":"10.1", "samsung":"8.2" }

https://github.com/zloirock/core-js/blob/master/packages/core-js-compat/src/data.js#L705

Chrome 61 (same version that first supports esmodules) V8 has a bug, probably not fixable w/ a code transform but don't know yet
- Makes me thing a motherfnBugfixes: true for arcane crap like this might be good...
Edge 16 because not explicitly given in core-js-compat, so all versions of Edge apply ...
FF 60, idk why b/c caniuse has marks y at 29
ios, safari again IDK, caniuse has safari mark y at 7.1 ...

connorjclark · 2020-06-10T06:38:40Z

Also, I have some catching up to do on the babel polyfill space :) babel/babel#11700 (comment)

It seems the logic for determining what polyfills are needed is being pulled up to a new plugin @babel/polyfills–instead of what we have today which is core-js@3 doing it's own thing. I'll need to look into how the babel plugin is determining what polyfills are needed. I hope its process has more attribution than core-js does.

@hzoo I understand this stuff is experimental atm, just wondering if the plan is to later modify @babel/preset-env to use any polyfill provider, or if the plan is to require @babel/preset-env for transforms (no core-js) and @babel/polyfills for polyfills.

nicolo-ribaudo · 2020-06-10T07:52:58Z

The plan is to keep the polyfills and the transforms separated, so eventually everyone will need to use one of the new polyfill providers.

On the other hand, we are exploring the idea moving back preset-env into @babel/core as a long-term goal,so you might have to only enable the polyfill provider plugin in the future.

If you are 100% sure that the typed arrays bug doesn't affect your code, you can use the exclude option to forcefully disable the polyfills.

connorjclark · 2020-06-10T23:32:36Z

The plan is to keep the polyfills and the transforms separated, so eventually everyone will need to use one of the new polyfill providers.
On the other hand, we are exploring the idea moving back preset-env into @babel/core as a long-term goal, so you might have to only enable the polyfill provider plugin in the future.

Interesting, thanks for the insight.

If you are 100% sure that the typed arrays bug doesn't affect your code, you can use the exclude option to forcefully disable the polyfills.

Thanks @nicolo-ribaudo . The goal of this audit ultimately is to recommend other developers to use preset-modules and esmodules to create two bundles and use the module/nomodule pattern. In our documentation we'll also recommend bugfixes: true.

We could go further and say "exclude typed polyfills" because the bug is seemingly so negligible yet the polyfill so large. We'd much rather explore the possibility of getting that to be the default behavior of esmoudles: true bugfixes: true (much wider reach). But using excludes as part of our recommended pattern might be a nice short-term solution ... (#10945).

nicolo-ribaudo · 2020-06-11T06:38:20Z

Also note that currently preset-modules should now have the same behaviour as preset-env { bufgixes: true, esmodules: true }, so you don't need to recommend both.

patrickhulce

sad day but important 👍

patrickhulce · 2020-06-11T19:59:20Z

lighthouse-core/audits/legacy-javascript.js

      ['Array.prototype.fill', 'es6.array.fill'],
      ['Array.prototype.filter', 'es6.array.filter'],
      ['Array.prototype.find', 'es6.array.find'],
      ['Array.prototype.findIndex', 'es6.array.find-index'],
      ['Array.prototype.forEach', 'es6.array.for-each'],
      ['Array.from', 'es6.array.from'],
      ['Array.isArray', 'es6.array.is-array'],
-      ['Array.prototype.lastIndexOf', 'es6.array.last-index-of'],


instead of removing these can we flag them with an extra property or move them to another array somewhere? it'd be nice to keep track of what we should detect once we figure out how to solve the bugfixes problem and not lose all this research

what we should detect is simply any polyfill that will never be in a bundle w/ esmodules: true, bugfixes: true (using either core-js@2 or core-js@3). everything being removed fails that criteria. if we add any back, it'll be because something changed upstream (and we can just add back to this list) or we accompany it with documentation for how and why to exclude (what we may do for typed-array, promise).

so I'm ok w/ just deleting these items. git blame and these PRs are enough history for me if I need to refer back for some reason.

This list is derived from the diff of the debug output when you toggle esmodule: true/false. It may be good to print out that output in legacy-javascript/run.js (doing much more than that would be more work than I'd like)

sidenote on the line you commented on... last-index-of used to be set to an older version of safari but was bumped here: zloirock/core-js@e4cea55

no idea what bug this is preventing lol https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/array-method-uses-to-length.js

alright wfm

I'd think future me would be annoyed by having to hunt down an ancient PR and copy paste individual lines from a diff that no longer merges for this info but I'll defer to your future self since it's almost definitely going to be you anyway :)

patrickhulce · 2020-06-11T20:05:33Z

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

-    }, /** @type {string[]} */ ([])).join(', ');
-    variants.push({name: dir, signals});
+
+    const signals = [];


it did fail but didn't exit with code 1

https://travis-ci.org/github/GoogleChrome/lighthouse/builds/696156441#L1224

need to add

main().catch(err => { console.error(err); process.exit(1); })

patrickhulce · 2020-06-11T20:06:59Z

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

@@ -1,29 +1,11 @@
 all-legacy-polyfills
-  175764       all-legacy-polyfills-core-js-3/main.bundle.min.js
-  125257       all-legacy-polyfills-core-js-2/main.bundle.min.js
+  85169        all-legacy-polyfills-core-js-3/main.bundle.min.js


ouch, takin' a 50% hit to the ceiling of benefit :(

patrickhulce · 2020-06-11T20:08:00Z

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

-      'WeakMap = function() {}',
-      'window.WeakMap = function() {}',
-      'function WeakMap() {}',
+      // 'WeakMap = function() {}',


should we delete these or are these comments important?

Did you see my comment a few lines up

I did, I just didn't make the connection :)

maybe just group all 4 of them together with that comment?

patrickhulce

LGTM

patrickhulce · 2020-06-12T19:49:11Z

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

-      'WeakMap = function() {}',
-      'window.WeakMap = function() {}',
-      'function WeakMap() {}',
+      // 'WeakMap = function() {}',


I did, I just didn't make the connection :)

maybe just group all 4 of them together with that comment?

patrickhulce · 2020-06-12T19:52:08Z

lighthouse-core/audits/legacy-javascript.js

      ['Array.prototype.fill', 'es6.array.fill'],
      ['Array.prototype.filter', 'es6.array.filter'],
      ['Array.prototype.find', 'es6.array.find'],
      ['Array.prototype.findIndex', 'es6.array.find-index'],
      ['Array.prototype.forEach', 'es6.array.for-each'],
      ['Array.from', 'es6.array.from'],
      ['Array.isArray', 'es6.array.is-array'],
-      ['Array.prototype.lastIndexOf', 'es6.array.last-index-of'],


alright wfm

I'd think future me would be annoyed by having to hunt down an ancient PR and copy paste individual lines from a diff that no longer merges for this info but I'll defer to your future self since it's almost definitely going to be you anyway :)

hzoo · 2020-06-15T15:21:20Z

I wish we had a better way to track each of these bugs, what they are/why you need them and/or an even more modern delivery discriminant than esmodules

@paulirish and I want to explore extending preset-modules to reduce the polyfill footprint. AFAIK it currently only reduces the number of transforms necessary. seems that typed-array may be plausible if it's really just b/c of toString.

I think we would be glad to chat or work with y'all on this!

…GoogleChrome#10937)

zloirock · 2021-11-29T03:12:35Z

If next time you will have questions about what and why polyfilled, please, ask me or at least take a look at tests that generate core-js-compat.

connorjclark added 4 commits June 9, 2020 17:39

initial

3743b93

rm polys

32efc63

rm object create

8a210c9

update

f1323fa

connorjclark requested a review from a team as a code owner June 10, 2020 01:45

connorjclark requested review from patrickhulce and removed request for a team June 10, 2020 01:45

googlebot added the cla: yes label Jun 10, 2020

devtools-bot assigned patrickhulce Jun 10, 2020

devtools-bot added the waiting4reviewer label Jun 10, 2020

connorjclark commented Jun 10, 2020

View reviewed changes

oops

b6a208e

vercel bot deployed to Preview June 10, 2020 01:47 View deployment

connorjclark commented Jun 10, 2020

View reviewed changes

patrickhulce reviewed Jun 10, 2020

View reviewed changes

connorjclark added the needs-discussion label Jun 10, 2020

connorjclark mentioned this pull request Jun 11, 2020

For legacy-javascript advice, suggest excluding some polyfills for "modern" build #10945

Open

connorjclark removed the needs-discussion label Jun 11, 2020

patrickhulce suggested changes Jun 11, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jun 11, 2020

connorjclark mentioned this pull request Jun 11, 2020

tests(legacy-javascript): exit code 1 on failure #10946

Merged

connorjclark changed the title ~~core(legacy-javascript): fix core-js import in test, reduce polyfills~~ core(legacy-javascript): reduce polyfills, fix core-js import in test Jun 12, 2020

patrickhulce approved these changes Jun 12, 2020

View reviewed changes

comment

51e66f3

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

connorjclark added land-when-ci-is-green and removed waiting4committer labels Jun 12, 2020

devtools-bot merged commit 24b9c7c into master Jun 12, 2020

devtools-bot deleted the leg-js-preset-env-esmodules branch June 12, 2020 23:46

devtools-bot removed the land-when-ci-is-green label Jun 12, 2020

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

core(legacy-javascript): reduce polyfills, fix core-js import in test (…

8e6b019

…GoogleChrome#10937)

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

core(legacy-javascript): reduce polyfills, fix core-js import in test (…

7a87e1c

…GoogleChrome#10937)

connorjclark mentioned this pull request Nov 29, 2021

Some JS polyfills should be added to the list of legacy JS #13440

Open

		['Number.parseInt', 'es6.number.parse-int'],
		['Object.assign', 'es6.object.assign'],

core(legacy-javascript): reduce polyfills, fix core-js import in test #10937

core(legacy-javascript): reduce polyfills, fix core-js import in test #10937

Conversation

connorjclark commented Jun 10, 2020 • edited by patrickhulce Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jun 10, 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

connorjclark Jun 10, 2020 • edited Loading

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

connorjclark Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jun 10, 2020 • edited Loading

connorjclark commented Jun 10, 2020

nicolo-ribaudo commented Jun 10, 2020 • edited Loading

connorjclark commented Jun 10, 2020 • edited Loading

nicolo-ribaudo commented Jun 11, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jun 11, 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

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

hzoo commented Jun 15, 2020

zloirock commented Nov 29, 2021

connorjclark commented Jun 10, 2020 •

edited by patrickhulce

Loading

connorjclark Jun 10, 2020 •

edited

Loading

connorjclark Jun 10, 2020 •

edited

Loading

connorjclark Jun 10, 2020 •

edited

Loading

connorjclark commented Jun 10, 2020 •

edited

Loading

nicolo-ribaudo commented Jun 10, 2020 •

edited

Loading

connorjclark commented Jun 10, 2020 •

edited

Loading

connorjclark Jun 11, 2020 •

edited

Loading