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

Configure babel to not transpile ES6 modules to CommonJS #688

Merged
merged 6 commits into from
Aug 17, 2018

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Jul 21, 2018

Fixes #683

Description

This changes the default configuration for babel passed from babel-loader such that the modules option is set to false, as well as some other changes to webpack configuration to ensure that ES modules are used instead of CommonJS modules wherever possible.

Changes/Tasks

  • Added a modules option to the built in babel preset. This is necessary because it must be undefined when running the node scripts, since node does not support ES modules.
  • Change jsLoader such that the babel preset is applied with the modules option set to false
  • Change module.strictExportPresence to true in webpack configs. This causes the import of an identifier that is not exported to be an error rather than a warning.
  • Remove the custom setting for resolve.mainFields in the webpack configs. This has the effect of changing the custom value ['browser', 'main'] to the current webpack default of ['browser', 'module', 'main']. This means that ES imports are also used for packages which include the standard module field in package.json.
  • Change resolve.extensions in webpack config from ['.js', '.json', '.jsx'] to ['.wasm', '.mjs', '.js', '.json', '.jsx']. This reflects the current webpack default with an added .jsx option. It has the effect of prioritising .mjs files, which generally have ES exports instead of CommonJS.

Motivation and Context

See #683

  • When the user imports an identifier from an ES6 module that is not exported, webpack throws an error. Previously, there was no error, and instead at runtime the imported variable would be undefined. This includes both user code (provided they use ES6 imports/exports), and packages (provided the package provides ES modules in the module field of package.json). This means the developer gets promptly warned about import errors during compilation instead of experiencing confusing errors in testing or production.
  • Webpack tree shaking works, since webpack is aware of the modules. This can lead to smaller bundle size.

Screenshots (if appropriate):

Types of changes

  • Refactoring/add tests (refactoring or adding test which isn't a fix or add a feature)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • My changes have tests around them

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Jul 21, 2018

@tannerlinsley after implementing this I had a few thoughts about what we discussed in #683

  • It's not necessary to apply babel-loader to node_modules to get the ES6 imports to work (instead its necessary to change the webpack config to use the ES modules that packages provide)
  • I looked though my projects where I've made a similar changes, and I can't find any packages that I had to transpile to commonJS apart from react-native specific packages (I suspect metro has different rules to webpack about mixing ES imports with CommonJS exports in the same file, which is what causes a problem). Many of those packages have since been updated to use ES exports anyway. Generally web packages have a pkg.main which uses CommonJS imports/exports, and pkg.module which uses ES imports/exports. These both work fine. Perhaps there is no need to provide a new option to the user?

@tannerlinsley
Copy link
Contributor

This is great. What about the circumstance where a user consumes babel-preset from their project's .babelrc IMO, they shouldn't have to set modules to false, they should only have to add it as a plugin (as it is in many of the projects already). Thoughts?

@hedgepigdaniel
Copy link
Contributor Author

hedgepigdaniel commented Jul 21, 2018

That case works (uses ES modules), because the options for the @babel/preset-env preset passed to babel-loader override those in the .babelrc file in the project.

In a project, the following .babelrc file contents work and use ES modules:

{
  "extends": "react-static/.babelrc",
}
{
  "presets": ["react-static/babel-preset"],
}

A difficulty is that since node does not support ES modules, its necessary to transform them to CommonJS on the compiler side. All the code that runs/configures webpack, does SSR etc can't have ES modules in it otherwise node will crash.

For example, this makes the build crash:

{
  "presets": [["react-static/babel-preset", { "modules": false }]],
}

If I understand babel/babel#6905 correctly (also https://babeljs.io/blog/2017/12/27/nearing-the-7.0-release#the-env-option-in-babelrc-is-not-being-deprecated), users can still add other presets in their .babelrc file and they will still work, but unless the user uses the new preset renaming syntax in their .babelrc, there will be only one instance of the @babel/preset-env in the config used in babel-loader, and it will always have the options { "modules": false }, nomatter what they put in their .babelrc.

Another option would be to use the envName babel option (which I assume works with babel-loader). jsLoader could set { envName: 'webpack' } and not override any presets - instead react-static/.babelrc could include the appropriate defaults based on that env name. That way the user would have an easier time configuring babel if they wanted to change the options for @babel/preset-env or not use it at all. They could do this by using the "env" option in .babelrc. I'm not sure how that would fit with the way REACT_STATIC_ENV/BABEL_ENV currently works though - since you'd want different settings for the build process vs compiled code, which is a different distinction to production/development

@tannerlinsley tannerlinsley merged commit dd508cb into react-static:master Aug 17, 2018
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.

Harmony/ES6 modules are unecessarily transpiled to CommonJS
2 participants