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: search for browserslist if esmodules is falsy #11124
Conversation
@@ -189,7 +189,8 @@ export default function getTargets( | |||
// Parse browsers target via browserslist | |||
const browsersquery = validateBrowsers(targets.browsers); | |||
|
|||
const hasTargets = Object.keys(targets).length > 0; | |||
const hasTargets = targets.esmodules || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should normalize target { esmodules: false }
to {}
in
const normalizeTargets = targets => { |
so if target.esmodules
is accessed later, if must be true
and we don't have to worry that it can be false
or any other fancy falsy values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...what I worry about is that some package use babel-helper-compilation-targets
directly, e.g. @vue/babel-preset-app. should we fix the bug for those packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Since target.esmodules
is used as a shortcut to set target.browsers
to be those with esmoduels support. We can delete it after it is consumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable ❤️. Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -184,6 +184,9 @@ export default function getTargets( | |||
targets.browsers = Object.keys(supportsESModules) | |||
.map(browser => `${browser} ${supportsESModules[browser]}`) | |||
.join(", "); | |||
} else { | |||
// remove falsy esmodules to fix `hasTargets` below | |||
delete targets.esmodules; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also delete it if it is truthy, since at this point we have already handled it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User may invoke this method twice or more, keep the truthy value is safer for this case
const getTargets = require( '@babel/helper-compilation-targets' ).default
const targets = { esmodules: true }
const result1 = getTargets( targets )
const result2 = getTargets( targets )
// result1 should be equal to result2 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't they still be the same?
After the first call, targets has a browsers
property which makes its behaviour identical to esmodules: true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry I didn't realize targets.browsers have already been assigned to targets. Updated
Thanks! |
As stated in issue #11123, with targets
babel won't search for browserslist config