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

Add numeric separator to shippedProposals #10971

Merged
merged 8 commits into from Mar 16, 2020

Conversation

Wetinee
Copy link
Contributor

@Wetinee Wetinee commented Jan 9, 2020

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

I've added numeric separator in babel-preset-env and babel-preset-dev-env, but failed some tests and I haven't found out why. Your help is greatly appreciated.

@JLHwung JLHwung added pkg: preset-env PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jan 9, 2020
@@ -9,6 +9,7 @@ Using modules transform: auto

Using plugins:
syntax-async-generators { "chrome":"71" }
proposal-numeric-separator { "chrome":"71" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When corejs.proposals is true, we should not add proposal-numeric-separator here because corejs.proposals should not imply shippedProposals: true.

@Wetinee Wetinee marked this pull request as ready for review January 9, 2020 05:50
TopLevelOptions.shippedProposals,
opts.shippedProposals,
false,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Practically it is not a breaking change, because we have never added any plugins to shippedProposals. So even if corejs.proposals implies shippedProposals, our user can not observe the effect. I would consider this change as bugfix.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.9.0 milestone Feb 23, 2020
@nicolo-ribaudo
Copy link
Member

The failing e2e test is fixed on master

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e2e test error is related: https://app.circleci.com/jobs/github/babel/babel/17386

Blocking this PR until the test error is fixed.

@JLHwung
Copy link
Contributor

JLHwung commented Feb 26, 2020

@Wetinee Because this is a new feature, can you also update https://babeljs.io/docs/en/babel-preset-env#shippedproposals and submit a PR? Here is the link to docs source.

@nicolo-ribaudo
Copy link
Member

@Wetinee The failure happens when using an old version of @babel/preset-env without numeric-separator support with a new version of @babel/compat-data which includes the plugin.

Right after importing pluginsList from @babel/compat-data, we should filter it and only keep what is defined in available-plugins.

@nicolo-ribaudo
Copy link
Member

The CI error is fixed by v7.8.7

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Mar 5, 2020
nicksellen added a commit to karrot-dev/karrot-frontend that referenced this pull request May 4, 2020
Storybook otherwise had a broken/incompatible version and we end
up with this error in CI:
https://app.circleci.com/pipelines/github/yunity/karrot-frontend/166/workflows/d7664456-b80f-4cc0-960f-07164329f038/jobs/24730

I think this is the related babel fix babel/babel#10971

We should remove this in the future, or solve it without forcing
resolutions.
tiltec pushed a commit to karrot-dev/karrot-frontend that referenced this pull request May 9, 2020
Storybook otherwise had a broken/incompatible version and we end
up with this error in CI:
https://app.circleci.com/pipelines/github/yunity/karrot-frontend/166/workflows/d7664456-b80f-4cc0-960f-07164329f038/jobs/24730

I think this is the related babel fix babel/babel#10971

We should remove this in the future, or solve it without forcing
resolutions.
@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 Jun 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: preset-env PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add numeric separator to shippedProposals in preset-env
4 participants