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

Fix babel config file detection #738

Merged

Conversation

jdreesen
Copy link
Contributor

Since v7 Babel supports two types of config formats:

  • Project-wide configuration
    • babel.config files, with the different extensions (json, js, cjs, mjs)
  • File-relative configuration
    • .babelrc files, with the different extensions (none, json, js, cjs, mjs)
    • package.json files with a "babel" key

Encore, however, only checks for the existence of file-relative config files.

This is a problem if you want to compile packages from the node_modules folder (via .configureBabel(null, {includeNodeModules: ['...']})) because project-wide configuration files are needed for this.

This means that if you only have a .babelrc.js file, the package from the node_modules folder will be compiled by Babel, but without the settings defined in the .babelrc.js file.

If you now rename the .babelrc.js to babel.config.js, Encore fails to detect it and applies its default Babel config, which results in everything being complied without the settings defined in babel.config.js.

Note: Configuring babel through Encore is not an option for me, because I want to make use of Babels Config Function API which is not available in Encore afaik.
And apart from that I prefer to have the configuration in dedicated files.

So my solution for now is to keep an empty .babelrc.js to make Encore think there is a Babel config file, and have an additional babel.config.js file with the real config. This results in both the application code and node modules being compiled with the desired settings.


But of course I dug a little deeper into Encore and Babel to fix the problem and the fix is using the hasFilesystemConfig() method instead of checking the babelrc property (the wording in the comment is a bit outdated, I already created a PR for that ;)):
https://github.com/babel/babel/blob/af669297efd775016725ee39318cdb391cf00a21/packages/babel-core/src/config/partial.js#L191-L200

I added some tests, too. I'm not sure if it's worth it to add tests for the different config file extensions, though.

I tried to improve the error messages as well. Tell me if you want another wording or different file examples and I'll change it.

@jdreesen jdreesen force-pushed the fix-babel-config-file-detection branch from 57334ca to a230e3a Compare April 24, 2020 11:56
@jdreesen
Copy link
Contributor Author

This may be related to #558 and #670 (comment).

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This is an incredible PR. I can see your comment fix on babel was already merged. You made minimal changes to Encore, they make sense, and you added tests. One of my favorite PR's :).

@weaverryan weaverryan force-pushed the fix-babel-config-file-detection branch from a230e3a to 5ce489e Compare May 6, 2020 14:09
@weaverryan
Copy link
Member

Thank you Jacob!

@weaverryan weaverryan merged commit 407d81e into symfony:master May 6, 2020
@ankurk91
Copy link

ankurk91 commented May 6, 2020

We could use cosmicconfig here, example:

const cosmiconfig = require('cosmiconfig');

const userConfigExists = () => {
  return !!cosmiconfig('babel', {
    searchPlaces: [
      'package.json',
      '.babelrc',
      '.babelrc.js',
      'babel.config.js'
    ]
  }).searchSync()
};

This package can used to search for postcss configs as well.

@Kocal
Copy link
Member

Kocal commented May 6, 2020

That's a quite solution, but I think it's better to let the package find its own configuration itself. This way we don't have to maintain a list of search places.

@jdreesen jdreesen deleted the fix-babel-config-file-detection branch May 6, 2020 17:56
weaverryan added a commit that referenced this pull request May 11, 2020
This PR was merged into the master branch.

Discussion
----------

Fixing babel.config.js filename in message

Introduced in #738 - @jdreesen I think this was just a typo? Can you confirm that my PR is right?

Thanks!

Commits
-------

c163909 fixing babel.config.js filename in message
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.

4 participants