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

Class features loose should have precedence over preset-env #11634

Merged
merged 6 commits into from May 29, 2020

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 27, 2020

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

Please suggest me a better method to fix this if possible 😭

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories i: regression labels May 27, 2020
@babel-bot
Copy link
Collaborator

babel-bot commented May 27, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/23191/

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cc1e1c9:

Sandbox Source
nice-saha-ukqcm Configuration
condescending-rhodes-usyzm Configuration

if (loose) file.set(looseKey, file.get(looseKey) | feature);
if (
loose ===
"#__internal__@babel/preset-env__prefer-true-but-false-is-ok-if-it-prevents-an-error"

This comment was marked as off-topic.

…s.js


[skip ci]

Co-authored-by: Huáng Jùnliàng <jlhwung@gmail.com>
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.

Learned a good lesson when we have option constraints between plugins.

@existentialism
Copy link
Member

Thought about this a bit... I think this is fine. Can we possibly emit a warning when the loose mode from @babel/preset-env is overridden by another plugin's option?

@nicolo-ribaudo
Copy link
Member Author

@existentialism I have added the warning. You can see in the tests added in the last commit that it can still be explicitly silenced by explicitly enabling the plugins with the correct loose mode.

@existentialism
Copy link
Member

Is it possible to show them which plugin overrode?

I think something like:

Though the loose option was set to true in your @babel/preset-env config, it will not be used since the 'loose' mode option was set for {plugin name}. The loose option must be the same for @babel/plugin-proposal-class-properties, @babel/plugin-proposal-private-methods and @babel/plugin-proposal-private-property-in-object (when they are enabled).

Might be slightly more helpful in both letting them know what is going on, and how to fix it.

@nicolo-ribaudo
Copy link
Member Author

Better?

@existentialism
Copy link
Member

I think it is, you?

We might be able to shorten it, but thats more bikeshedding for later. I think it describes the problem well and how to fix it.

@nicolo-ribaudo
Copy link
Member Author

Yeah, we don't need to shorten it. It's more important to exactly explain what's going on and how to fix it.

@nicolo-ribaudo nicolo-ribaudo merged commit e6d873e into babel:master May 29, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the class-features-loose-env branch May 29, 2020 22:19
@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 Aug 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue 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.

Odd 'loose' mode configuration must be the same...
4 participants