Skip to content

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

Merged
nicolo-ribaudo merged 1 commit intobabel:masterfrom
haoqunjiang:fix-isPluginRequired-regression
Jan 12, 2020
Merged

fix: isPluginRequired returns the opposite result in v7.8.0#10992
nicolo-ribaudo merged 1 commit intobabel:masterfrom
haoqunjiang:fix-isPluginRequired-regression

Conversation

@haoqunjiang
Copy link
Copy Markdown
Contributor

@haoqunjiang haoqunjiang 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).

JLHwung
JLHwung approved these changes Jan 12, 2020
View reviewed changes
Copy link
Copy Markdown
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.

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:

babel/scripts/integration-tests/e2e-vue-cli.sh

Line 33 in 2ccc451

CI=true yarn test -p babel

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

@JLHwung JLHwung added i: regression PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: preset-env labels Jan 12, 2020
@haoqunjiang
Copy link
Copy Markdown
Contributor Author

haoqunjiang 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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jan 12, 2020

Published as 7.8.2!

@haoqunjiang
Copy link
Copy Markdown
Contributor Author

haoqunjiang commented Jan 13, 2020

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

@JLHwung JLHwung mentioned this pull request Jan 13, 2020
Merged
@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 Apr 13, 2020
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

Labels

i: regression 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@haoqunjiang @JLHwung @nicolo-ribaudo