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

Update preset-env builtin-definitions #10929

Merged
merged 10 commits into from Feb 17, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Dec 26, 2019

Q                       A
Fixed Issues? Fixes #10883
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

useBuiltIns: "usage" will now include core-js stage-4 proposals by default. Besides that, the corejs3/shippedProposals are now moved to the data/ folder and now generated from core-js-compat.

Ideally we should generate imports using standarized entry names, i.e. use es.global-this instead of esnext.global-this, but this will break users using an older version of core-js back when global-this hadn't reached stage-4 yet.

@JLHwung JLHwung added PR: New Feature 🚀 A type of pull request used for our changelog categories area: compat-table pkg: preset-env labels Dec 26, 2019
@nicolo-ribaudo
Copy link
Member

Ideally we should generate imports using standarized entry names, i.e. use es.global-this instead of esnext.global-this, but this will break users using an older version of core-js back when global-this hadn't reached stage-4 yet.

Maybe we could do it based on the corejs version option?

@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 27, 2019

@nicolo-ribaudo Well if we do it for the minors, we will need to keep track of which versions shipping which standard names. I think since it is not visible to users (as long as core-js still offers es.next entries), we can switch to standard entries on core-js major updates.

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.

but this will break users using an older version of core-js back when global-this hadn't reached stage-4 yet

Nope. All required logic already in the architecture. Need to update only built-ins definitions. For example, for globalThis should be added both - esnext.global-this and es.global-this - and will be used only modules for the specified core-js version (like corejs: '3.6' or corejs: '3.1').

@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 27, 2019

@zloirock Thanks, I just realize that we are doing sub imports for core-js-compat, so it is all handled by core-js-compat.

import getModulesListForTargetVersion from "core-js-compat/get-modules-list-for-target-version";

@JLHwung JLHwung force-pushed the update-corejs-features-json branch from b3f5744 to de42cfd Compare January 7, 2020 23:31
@zloirock
Copy link
Member

zloirock commented Jan 8, 2020

One more time: for enabling finished proposals should be updated only definitions, we should not return conception of shipped proposals for finished proposals to core-js plugins. Users should not use corejs: 3 and have enabled proposals marked as finished in the latest core-js versions - they should use, for example, corejs: '3.6' - there are many other modules which were added in the latest versions and missed in early versions, some features, like String#matchAll have a serious difference as a proposal in early core-js@3 versions and as a stable ES feature in the actual core-js - if it will work "at least somehow" users will use corejs: 3 and miss many other required modules. It would be much better to improve documentation for recommendation specifying minor core-js version in the config. Also, many other modules were added in minor core-js@3 versions, not just those 3 - they should be added too.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 8, 2020

for enabling finished proposals should be updated only definitions, we should not return conception of shipped proposals for finished proposals to core-js plugins.

I agree that shipped proposals should exclude finished ones. But if we do it now we will introduce breaking change for users using an old core-js version. Like we will do in babel-parser, we can remove these 3 finished proposals in babel 8.

they should use, for example, corejs: '3.6'

Yep. We should update documents and encourage the minor version specification. We can add a deprecation message for 2 and 3 in babel 8.

Also, many other modules were added in minor core-js@3 versions, not just those 3 - they should be added too.

Yep. I will come up with a new script to remind maintainers that built-in-definitions needs to be updated. But that should be addressed in another PR.

@zloirock
Copy link
Member

zloirock commented Jan 8, 2020

But if we do it now we will introduce breaking change for users using an old core-js version...

It was a comment mainly about extending the current shipped proposals built-ins list only by finished proposals. If we wanna maintain proper "shipped proposals" functionality - it should be not just finished proposals, it could be the list of modules for stage 3+ proposals which we could get as require('core-js-compat/entries')['core-js/stage/3'].

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 8, 2020

The shippedProposals does not include the same feature set of stage-3, because stage-3 entrance does not require compatible implementations and the spec could change before landing to browsers.

We are now testing if a proposal is shipped by querying core-js-compat that if a feature has non-empty browser data. It happens to only include 3 finished proposals now. That said, if any stage 3 features are implemented and core-js-compat provides the support data, it will be included in shippedProposals.json.

@JLHwung JLHwung changed the title Include corejs finished proposals by default update preset-env builtin-definitions Jan 8, 2020
@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories and removed PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jan 8, 2020
@zloirock
Copy link
Member

zloirock commented Jan 8, 2020

I know what is shippedProposals and almost all time it's the same that stage 3 since now stage 3 is a marker that it should be implemented in browsers. The difference was some years ago. As you can see, String#replaceAll and Promise.any are implemented in some browsers (maybe in nightly and under the flags, but implemented) - so it should be in shippedProposals too and it's the same as Stage 3+.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 8, 2020

maybe in nightly and under the flags, but implemented

Yeah. Both String#replaceAll and Promise.any are behind a flag or available at TP only. That may be why core-js-compat does not provide browser support data for them.

So here is some different interpretations of shippedProposals: Does shipped includes those flag-guarded or only features with general availability? Personally I prefer the latter because they are more mature and we can opt in these features in preset-env.

@nicolo-ribaudo
Copy link
Member

I also agree that shippedProposals should mean "available unflagged in stable releases", which is the same criteria that TC39 uses to decide when a proposal can be advanced to stage 4. It's like Stage 3½.

@zloirock
Copy link
Member

zloirock commented Jan 9, 2020

I could accept your position. However, it will break shippedProposals. For example:

  • Sometimes proposals have changes on stage 3 or even after adding to the spec. For example, String#matchAll. After that, compat data for related modules discarded. So, they will be removed from shippedProposals.
  • Some features are available in browsers even on stage 0. For example, URL and URLSearchParams, but at this moment it's not a problem for us since it's also in the web. namespace. However, we had precedents like this before (String#{ trimLeft, trimRight }, global, etc.) and we will have it in the future.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 17, 2020

@zloirock I acknowledge your concern on this approach that detecting browser data may not exactly match the shippedProposal goal, as @nicolo-ribaudo said, Stage 3½. I want to clarify that this script is not the ultimate reference for corejs3-shippedProposals.json, it is meant to remind the contributors that some feature X are now shipped in some engine Y and we might need to update the builtin definition. It helps bring up the otherwise unaware issue, and is still at reviewers' discretion that the changes shall be merged.

As for your concern, we can certainly introduce whitelist in the future if a browser were shipping < state-3 proposals (usually something private is now standardized) or compat-data have been truncated.

@nicolo-ribaudo
Copy link
Member

@JLHwung Has #10929 (review) (esnext.* + es.*) been addressed? Or is it not necessary?

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 4, 2020

@nicolo-ribaudo Yeah, it has been addressed in f754938.

@nicolo-ribaudo nicolo-ribaudo dismissed zloirock’s stale review February 5, 2020 23:56

The requested changes have been addressed (#10929 (comment))

@nicolo-ribaudo
Copy link
Member

@JLHwung v7.9.0 or v7.8.6?

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 17, 2020

@nicolo-ribaudo I prefer to ship it in patch release, same as what we did when we bumped browserslist.

@nicolo-ribaudo nicolo-ribaudo changed the title update preset-env builtin-definitions Update preset-env builtin-definitions Feb 17, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 1613418 into babel:master Feb 17, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the update-corejs-features-json branch February 17, 2020 18:30
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: compat-table outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: preset-env PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not importing Promise.allSettled with useBuiltIns: 'usage'
4 participants