Skip to content
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

Merged
merged 3 commits into from Mar 29, 2019

Conversation

Lyrkan
Copy link
Collaborator

@Lyrkan Lyrkan commented Mar 21, 2019

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
// 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'],
});

@Lyrkan
Copy link
Collaborator Author

Lyrkan commented Mar 21, 2019

Test failure is unrelated, it seems that Babel is now displaying a warning when useBuiltIns is set (which is the case by default in Encore) without also setting the corejs option.

See: babel/babel#7646

index.js Outdated Show resolved Hide resolved
lib/WebpackConfig.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Lyrkan Lyrkan force-pushed the configure-babel-optional-callback branch from 53901c0 to 644d1ac Compare March 26, 2019 09:11
index.js Outdated
*
* @param {function} callback
* @param {function|false} callback
Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

weaverryan added a commit that referenced this pull request Mar 29, 2019
… 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
@weaverryan
Copy link
Member

Thanks @Lyrkan!

@weaverryan weaverryan merged commit a9af955 into symfony:master Mar 29, 2019
weaverryan added a commit that referenced this pull request Mar 29, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuring the babel loader is not possible when using babelrc file
3 participants