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
Add .babelrc error to configureBabelPresetEnv and remove some deprecations #651
Conversation
a58bc1a
to
5154b38
Compare
@@ -405,10 +405,6 @@ class WebpackConfig { | |||
normalizedOptionKey = 'includeNodeModules'; | |||
} | |||
|
|||
if (['useBuiltIns', 'corejs'].includes(optionKey)) { |
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.
If we don't deprecate the options, it's possible to do:
Encore
.configureBabel(null, {
corejs: 3,
})
.configureBabelPresetEnv((options) => {
options.corejs: 2
});
I think it's better to update the flex recipe instead of not deprecating the options because the recipe recommends them.
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.
If we don't deprecate the options, it's possible to do:
Encore .configureBabel(null, { corejs: 3, }) .configureBabelPresetEnv((options) => { options.corejs: 2 });
I don't think that's a real issue, the options have the same name and it's quite unlikely someone will try to set them in both configureBabel()
and configureBabelPresetEnv()
(and even more unlikely that they will with different values).
That's also the case for other methods in Encore, for instance if you call Encore.enableVueLoader(() => {}, { useJsx: true })
, and Encore.configureBabel(options => { options.presets = []; })
.
I think it's better to update the flex recipe instead of not deprecating the options because the recipe recommends them.
It doesn't really cost us anything to keep them here, but it will cost time to users that initialized Encore using the flex recipe if we remove them.
But as I said that's not the only reason: I think having some real options that we can document / add checks on beside callbacks is a good thing. Maybe it could be argued that in this case they should be set by configureBabelPresetEnv()
but that's not a big issue either (it's still related to Babel after all).
Thank you @Lyrkan. |
…ome deprecations (Lyrkan) This PR was merged into the master branch. Discussion ---------- Add .babelrc error to configureBabelPresetEnv and remove some deprecations Since we do not configure `@babel/preset-env` if there is a `.babelrc` file (or something similar) it makes sense to add the same warning we already have for the `Encore.configureBabel()` function to `Encore.configureBabelPresetEnv()`: > The "callback" argument of \<function\> will not be used because your app already provides an external Babel configuration (a ".babelrc" file, ".babelrc.js" file or "babel" key in "package.json This PR also removes some deprecation that were added by #642. We talked about them with @weaverryan and it's probably not worth removing the `useBuiltIns` and `corejs` options of `Encore.configureBabel()`: * they are currently recommended by the flex recipe * it allows us to document them/check them easier than with a callback Commits ------- 5154b38 Add .babelrc warning to configureBabelPresetEnv and remove some deprecations
Since we do not configure
@babel/preset-env
if there is a.babelrc
file (or something similar) it makes sense to add the same warning we already have for theEncore.configureBabel()
function toEncore.configureBabelPresetEnv()
:This PR also removes some deprecation that were added by #642.
We talked about them with @weaverryan and it's probably not worth removing the
useBuiltIns
andcorejs
options ofEncore.configureBabel()
: