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: Don't load browserslist in block-hoist-plugin #13182
fix: Don't load browserslist in block-hoist-plugin #13182
Conversation
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 0e11cca:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45489/ |
@@ -12,6 +12,7 @@ export default function loadBlockHoistPlugin(): Plugin { | |||
const config = loadConfig.sync({ | |||
babelrc: false, | |||
configFile: false, | |||
browserslistConfigFile: false, |
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.
Actually we can completely avoid loadConfig.sync
. I drafted some perf changes before but never come up with a PR: JLHwung@15da88e
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.
That would even be better! It wasn't clear to me why it was needed so I tried to work around 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.
@JLHwung are you planning to submit your changes as a PR?
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.
Nope, feel free to tackle 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.
seems good as temp fix then? didn't even know we had this..
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.
The code changes look good.
If you have time to work on it I would prefer something like what @JLHwung proposed in #13182 (comment); otherwise we can merge this and then update it in the future.
7.13.0 introduced the new option
browserslistConfigFile
in #12189. The documentation specifies that the option toggles whatever babel looks for a browserslists configuration or not.This works fine, except that the
block-hoist-plugin
loads a configuration without setting thebrowserslistConfigFile
to false.The
block-hoist-plugin
should set thebrowserslistConfigFile
tofalse
to prevent babel from loading (or looking for) abrowserslists
config when users explicitly disabled this functionality. Theblock-hoist-plugin
makes no use of the browserslist itself, which is why the change should be fine.