-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Allow to call configureBabel with an external Babel configuration #544
Allow to call configureBabel with an external Babel configuration #544
Conversation
Test failure is unrelated, it seems that Babel is now displaying a warning when See: babel/babel#7646 |
53901c0
to
644d1ac
Compare
index.js
Outdated
* | ||
* @param {function} callback | ||
* @param {function|false} callback |
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 suggest documenting as function|null
rather than function|false
. Using a boolean to represent the absence of value does not make much sense.
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.
btw, the error message even says Argument 1 to configureBabel() must be a callback function or null.
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.
Yeah, I initially went with null
but for some reasons switched to false
(and didn't update that message).
I think my reasoning was something like "that parameter is disabled and won't be used", but now that you point it out I agree that it makes more sense to put null
.
… version (Lyrkan) This PR was merged into the master branch. Discussion ---------- Set useBuiltIns to false by default and allow to set core-js version While working on #544 I noticed that the latest version of `@babel/preset-env` displayed a warning (which luckily made [one of the tests](https://travis-ci.org/symfony/webpack-encore/jobs/509655320) fail): ``` WARNING: We noticed you're using the `useBuiltIns` option without declaring a core-js version. Currently, we assume version 2.x when no version is passed. Since this default version will likely change in future versions of Babel, we recommend explicitly setting the core-js version you are using via the `corejs` option. You should also be sure that the version you pass to the `corejs` option matches the version specified in your `package.json`'s `dependencies` section. If it doesn't, you need to run one of the following commands: npm install --save core-js@2 npm install --save core-js@3 yarn add core-js@2 yarn add core-js@3 ``` This is related to the `core-js` 3 update that happened recently (see babel/babel#7646). As explained in parcel-bundler/parcel#2819 (comment) the issue is that users are normally supposed to add `core-js` in their root `package.json` when setting `useBuiltIns` (which is currently already set by Encore to `entry` by default). This is a problem for us since it means that: * we are requiring users to add `core-js` to their project by default (or manually setting `useBuiltIns` to `false`) * if they do they'll still have a warning because the new `corejs` option of `@babel/preset-env` won't be set (we can't do that because we won't know which version they choose to install). For now I suggest that we set `useBuiltIns` to `false` by default and add a new option to `Encore.configureBabel()` that allows to set the `corejs` option. Commits ------- 85896ef Set useBuiltIns to false by default and allow to set corejs version
Thanks @Lyrkan! |
…iguration (Lyrkan, weaverryan) This PR was squashed before being merged into the master branch (closes #544). Discussion ---------- Allow to call configureBabel with an external Babel configuration This PR fixes #543 by allowing to call `Encore.configureBabel()` even if an external Babel configuration exists. Basically: * `null` (or any falsy value in reality, to keep it simple) is always allowed as a first argument * a warning will be displayed if the first argument is a callback and something like a `.babelrc` file is detected * a warning will be displayed if the second argument contains a non-whitelisted option (ie. not `includeNodeModules`/`exclude`) and something like a `.babelrc` file is detected ```js // Allowed without any warning Encore.configureBabel(null, { includeNodeModules: ['foo'] }); // Displays a warning and the // callback is ignored Encore.configureBabel( () => { /* ... */ } ); // Displays a warning and the // useBuiltIns option is ignored. Encore.configureBabel(null, { useBuiltIns: 'foo', includeNodeModules: ['foo'], }); ``` Commits ------- a9af955 Merge branch 'master' into configure-babel-optional-callback ffe3054 Recommend null instead of false for the first parameter of configureBabel() 644d1ac Allow to call configureBabel with an external Babel configuration
This PR fixes #543 by allowing to call
Encore.configureBabel()
even if an external Babel configuration exists.Basically:
null
(or any falsy value in reality, to keep it simple) is always allowed as a first argument.babelrc
file is detectedincludeNodeModules
/exclude
) and something like a.babelrc
file is detected