Skip to content
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

Merged
merged 24 commits into from
Oct 8, 2021

Conversation

JLHwung
Copy link
Collaborator

@JLHwung JLHwung commented Aug 24, 2021

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.

@JLHwung JLHwung added the enhancement New feature or request label Aug 24, 2021
@@ -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",
Copy link
Member

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.

@zloirock
Copy link
Member

It seems that shipped-proposals.js also should be updated.

@JLHwung JLHwung force-pushed the support-new-proposals-corejs3 branch from d82062a to 08ab606 Compare August 25, 2021 19:05
@JLHwung JLHwung force-pushed the support-new-proposals-corejs3 branch from 0a47593 to 2ec5b26 Compare August 26, 2021 13:48

const IteratorDependencies = [
"esnext.iterator.constructor",
...CommonIteratorsWithTag,
Copy link
Member

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.

Copy link
Collaborator Author

@JLHwung JLHwung Aug 27, 2021

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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",
Copy link
Member

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.

Copy link
Collaborator Author

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) }

Copy link
Member

@zloirock zloirock Aug 27, 2021

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)

Copy link
Member

@zloirock zloirock Aug 27, 2021

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Copy link
Member

@zloirock zloirock Aug 27, 2021

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 that es.map is required for .uniqueBy because of an esnext features which should only be enabled behind proposals: 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.

Copy link
Member

@zloirock zloirock left a 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).

@JLHwung
Copy link
Collaborator Author

JLHwung commented Aug 27, 2021

I didn't add annex B features because they are not recommended for general usage and no one ever complains about that. es.string.substr should be added though.

@zloirock
Copy link
Member

It's at first for legacy code - it's why they are in Annex B, but not removed from the spec.

@zloirock
Copy link
Member

Missed es.regexp.test (with es.regexp.exec dependency).

@zloirock
Copy link
Member

Left:

es.date.get-year
es.date.set-year
es.date.to-gmt-string
es.escape
es.unescape

es.reflect.to-string-tag

and updating of shipped-proposals.js.

@JLHwung
Copy link
Collaborator Author

JLHwung commented Aug 30, 2021

I tend to defer the annex-b support. Other review comments are resolved.

@zloirock
Copy link
Member

I insist on adding Annex B features at least for consistency with many other already added Annex B features. At least to the global version. Anyway, they will not be added if targets are configured properly and they are not required. If you use a legacy library that uses escape / unescape in an embed JS engine or .getYear in IE8, they should work as expected.

@zloirock
Copy link
Member

zloirock commented Sep 1, 2021

Please, update core-js-compat to 3.17.0.

@JLHwung JLHwung force-pushed the support-new-proposals-corejs3 branch from 2d2c4b2 to e2877e5 Compare September 20, 2021 14:38
@JLHwung JLHwung merged commit a537aa1 into babel:main Oct 8, 2021
@JLHwung JLHwung deleted the support-new-proposals-corejs3 branch October 8, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants