-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(legacy-javascript): fix core-js 3 detection #10852
Conversation
7550 es6-date-now/main.bundle.min.js | ||
6037 es6-date-to-string/main.bundle.min.js | ||
3454 es6-function-name/main.bundle.min.js | ||
47832 es6-typed-uint8-clamped-array/main.bundle.min.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizes changed because modules keys in the bundle are now numbers instead of strings.
7549 es6-date-now/main.bundle.min.js | ||
6036 es6-date-to-string/main.bundle.min.js | ||
3454 es6-function-name/main.bundle.min.js | ||
62423 es-typed-array-uint8-clamped-array/main.bundle.min.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizes changed because this now actually uses core js 3 :)
@@ -372,324 +354,324 @@ | |||
"signals": "" | |||
}, | |||
{ | |||
"name": "core-js-3-only-polyfill/es6-array-fill", | |||
"signals": "Array.prototype.fill" | |||
"name": "core-js-3-only-polyfill/es-array-buffer-constructor", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The misalignment in the diff is unfortunate, but no need to really review this. All the signals for core-js 3 variants before were simply wrong, this new patch should be consider a new baseline.
Looks like WeakMap
is used to help define the majority of this polyfils in core js 3, so it's in a lot of variants. for most of the "only-polyfill" variants there is another signal for the polyfill in isolation. Just a few are missing that, ndb.
return { | ||
coreJs2Module, | ||
coreJs3Module: coreJs2Module | ||
.replace('es6.', 'es.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a somewhat nice transformation from core js 2 -> core js 3 module names. I'm confident that all of these are accurate–otherwise the all-legacy-polyfills
variants would error during bundling.
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was surprising. core-js@3 bytes size increased ~40% for the set of "legacy" polyfills.
{...defineModules('es7.object.get-own-property-descriptors'), name: 'Object.getOwnPropertyDescriptors'}, | ||
{...defineModules('es7.object.values'), name: 'Object.values'}, | ||
{...defineModules('es7.string.pad-end'), name: 'String.prototype.padEnd'}, | ||
{...defineModules('es7.string.pad-start'), name: 'String.prototype.padStart'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by: a simpler array and then a .map()
into a array of {name: string, coreJs2Module: string, coreJs3Module: string}
might read more straightforwardly here.
Something like
static getPolyfillData() {
return [
/* eslint-disable max-len */
['Array.prototype.fill', 'es6.array.fill'],
['Array.prototype.filter', 'es6.array.filter'],
// ...
['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'},
{name: 'DataView', coreJs2Module: 'es6.typed.data-view', coreJs3Module: 'es.data-view'},
['Float32Array', 'es6.typed.float32-array'],
// ...
['String.prototype.padStart', 'es7.string.pad-start'],
/* eslint-enable max-len */
].map(info => {
if (!Array.isArray(info)) return info;
const [name, coreJs2Module] = info;
return {
name,
coreJs2Module,
coreJs3Module: coreJs2Module
.replace('es6.', 'es.')
.replace('es7.', 'es.')
.replace('typed.', 'typed-array.'),
};
});
}
(could also do an in
check or whatever instead of isArray
if want to keep named properties)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)
"variantsMissingSignals": [ | ||
"core-js-2-preset-env-esmodules/true", | ||
"core-js-3-preset-env-esmodules/true" | ||
], | ||
"variants": [ | ||
{ | ||
"name": "all-legacy-polyfills/all-legacy-polyfills-core-js-2", | ||
"signals": "Promise, Uint8Array, Object.getOwnPropertyDescriptor, ArrayBuffer, DataView, Array.prototype.fill, Array.prototype.findIndex, Array.prototype.find, Array.prototype.forEach, Array.from, Array.isArray, Array.prototype.lastIndexOf, Array.of, Date.now, Date.prototype.toISOString, Date.prototype.toJSON, Map, Number.isInteger, Number.isSafeInteger, Number.parseFloat, Number.parseInt, Object.assign, Object.create, Object.defineProperties, Object.defineProperty, Object.setPrototypeOf, Reflect.apply, Reflect.construct, Reflect.deleteProperty, Reflect.getOwnPropertyDescriptor, Reflect.getPrototypeOf, Reflect.get, Reflect.has, Reflect.isExtensible, Reflect.ownKeys, Reflect.preventExtensions, Reflect.setPrototypeOf, Reflect.set, Set, String.prototype.codePointAt, String.prototype.endsWith, String.fromCodePoint, String.prototype.includes, String.raw, String.prototype.repeat, String.prototype.startsWith, Float32Array, Float64Array, Int16Array, Int32Array, Int8Array, Uint16Array, Uint32Array, Uint8ClampedArray, WeakMap, WeakSet, Array.prototype.includes, Object.entries, Object.getOwnPropertyDescriptors, Object.values, String.prototype.padEnd, String.prototype.padStart, Array.prototype.filter, Array.prototype.map, Array.prototype.reduce, Array.prototype.reduceRight, Array.prototype.some, Date.prototype.toString, Function.prototype.name, Object.freeze, Object.getOwnPropertyNames, Object.getPrototypeOf, Object.isExtensible, Object.isFrozen, Object.isSealed, Object.keys, Object.preventExtensions, Object.seal, Reflect.defineProperty, String.prototype.trim" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna follow up with changing signals
to an array. different PR b/c it will be a large diff with no content changes.
These tests were mistakenly only checking core-js@2:
The result of this mistake was:
Changes to summary data:
all-polyfils
variant)all-polyfils
variant)