-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Support new proposals corejs3 polyfills #99
Conversation
@@ -483,6 +515,10 @@ export const StaticProperties: ObjectMap< | |||
}; | |||
|
|||
export const InstanceProperties = { | |||
asIndexedPairs: define("instance/asIndexedPairs", [ | |||
"esnext.async-iterator.as-indexed-pairs", | |||
"esnext.iterator.as-indexed-pairs", |
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 most core-js
modules have dependencies. Check related core-js
entry points like this https://github.com/zloirock/core-js/blob/master/packages/core-js/features/async-iterator/as-indexed-pairs.js.
It seems that |
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
d82062a
to
08ab606
Compare
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
0a47593
to
2ec5b26
Compare
|
||
const IteratorDependencies = [ | ||
"esnext.iterator.constructor", | ||
...CommonIteratorsWithTag, |
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.
Common iterators are required only for (Async)Iterator.from
, not for other methods from this proposal. Also, you didn't add (Async)IteratorDependencies
to some methods.
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.
you didn't add (Async)IteratorDependencies to some methods.
Yeah I should have explained that. Some (Async)IteratorDependencies
are not added because the plugin currently does not support dependencies of conditional dependencies. If we add (Async)IteratorDependencies
to .map
because of esnext.array-iterator.map
, users of es.array.map
will find es.promise
imported even though they do not enable proposals: true
, which will be very confusing.
We can add back these dependencies when the provider support this feature.
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.
However, those (async)iterators methods will not work without those dependencies. I solved it before by moving instance dependencies to constructors, the thread below, but in this case, we can't do it.
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.
However, those (async)iterators methods will not work without those dependencies.
In that case I lean to remove these features (not for take
which does not share name with stable features) and reland them after we support sub dependencies, otherwise it unnecessarily bloat the generated code, assuming most users only use stable features.
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.
It will work in combination with other features that have those dependencies, so it's better to leave them for work at least somehow.
@@ -573,6 +705,10 @@ export const InstanceProperties = { | |||
trimLeft: define("instance/trim-left", ["es.string.trim-start"]), | |||
trimRight: define("instance/trim-right", ["es.string.trim-end"]), | |||
trimStart: define("instance/trim-start", ["es.string.trim-start"]), | |||
uniqueBy: define("instance/unique-by", [ | |||
"esnext.array.unique-by", | |||
"esnext.typed-array.unique-by", |
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.
You forgot to remove esnext.typed-array.*
and *.emplace
from instance properties dependencies after moving them to constructors dependencies.
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 don't follow why they should be removed from instance properties. The constructor dependencies are only used in builtins such as Float32Array
via typed
helper. If we remove them from instance properties, we can't polyfill usages where the constructor does not present:
function (typeArray) { return typeArray.uniqueBy(x => x%2) }
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.
Before passing to those functions, typeArray
should be created and most likely via typed arrays constructors that have those dependencies. It was moved to constructors dependencies by the reason that you wrote here #99 (comment)
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.
Also, uniqueBy
depends on es.map
.
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.
Also, uniqueBy depends on es.map.
Another use case for sub dependencies. And if we remove esnext.array.unique-by
here, we lost the knowledge that es.map
is required for .uniqueBy
because of an esnext
features which should only be enabled behind proposals: true
.
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.
Or we may deliberately use the esnext.*
alias for stable features (if they will be supported in corejs3), so we trick the plugin to consider esnext.map
a proposal feature and don't pull esnext.map
for .uniqueBy
when proposals
are not enabled.
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.
And if we remove
esnext.array.unique-by
here, we lost the knowledge thates.map
is required for.uniqueBy
because of anesnext
features which should only be enabled behindproposals: true
.
I'm not sure that I understood what you mean.
Yes, only es.map
will be added without proposals: true
on .uniqueBy
. But .uniqueBy
is not too common property name. es.map
should be added only on .uniqueBy
instance property, so it will not pollute the bundle only with typed array creation.
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
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.
Missed definitions for Annex B features:
es.date.get-year
es.date.set-year
es.date.to-gmt-string
es.escape
es.string.substr
es.unescape
They are required at first for non-browser engines and IE8-.
Missed es.reflect.to-string-tag
that should be added to the global Reflect
dependency (+ es.object.to-string
).
I didn't add annex B features because they are not recommended for general usage and no one ever complains about that. |
It's at first for legacy code - it's why they are in Annex B, but not removed from the spec. |
Missed |
Left:
and updating of |
I tend to defer the annex-b support. Other review comments are resolved. |
I insist on adding Annex B features at least for consistency with many other already added Annex B features. At least to the |
packages/babel-plugin-polyfill-corejs3/src/built-in-definitions.js
Outdated
Show resolved
Hide resolved
Please, update |
They are supported in constructor dependencies
2d2c4b2
to
e2877e5
Compare
In this PR we add all proposals currently supported in
core-js@3.18.0
.This PR includes commits from #98, I will rebase when that one gets merged.