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

fix: `isPluginRequired` returns the opposite result in v7.8.0 #10992

Merged

Conversation

@sodatea
Copy link
Contributor

sodatea commented Jan 12, 2020

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

As far as I can tell, the new isRequired function in this PR https://github.com/babel/babel/pull/10899/files does the same job as the legacy isPluginRequired, so the ! here seems incorrect.

It breaks Vue CLI's tests at the moment (though it seems few people have noticed).

Copy link
Contributor

JLHwung left a comment

LGTM. Likely a glitch when refactoring targetsSupported into isRequired.

It breaks Vue CLI's tests at the moment

We do have vue-cli e2e tests:

CI=true yarn test -p babel

Could you post the broken CI link? It should have been captured by the e2e tests.

@sodatea

This comment has been minimized.

Copy link
Contributor Author

sodatea commented Jan 12, 2020

https://circleci.com/gh/sodatea/vue-cli/2224?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
I updated the lockfile in an unrelated PR and encountered this issue.

The e2e test script didn't include the preset package. Should be yarn test -p babel,babel-preset-app

@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Jan 12, 2020

@sodatea Thanks! Can you also update the babel/scripts/integration-tests/e2e-vue-cli.sh? So it can be captured in e2e tests.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 12, 2020

Will release in a few hours 👍

@nicolo-ribaudo nicolo-ribaudo merged commit f995f8e into babel:master Jan 12, 2020
4 of 5 checks passed
4 of 5 checks passed
build (13.x)
Details
test262 Workflow: test262
Details
Travis CI - Pull Request Build Passed
Details
build-standalone Workflow: build-standalone
Details
e2e Workflow: e2e
Details
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 12, 2020

Published as 7.8.2!

@sodatea

This comment has been minimized.

Copy link
Contributor Author

sodatea commented Jan 13, 2020

I went to sleep after the last reply 😅
Thanks for the quick action!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.