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

Add modules: false by default for es2015/env/latest presets #529

Closed
wants to merge 0 commits into from
Closed

Add modules: false by default for es2015/env/latest presets #529

wants to merge 0 commits into from

Conversation

SpaceK33z
Copy link

@SpaceK33z SpaceK33z commented Nov 4, 2017

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

See #521.

What is the new behavior?

The Babel presets env, latest and es2015 will be mutated to add modules: false as an option. This also works when you use e.g. @babel/preset-env or @babel/env.

Does this PR introduce a breaking change?

  • Yes
  • No

For people who use babel-loader without setting the modules option, there might be small differences in the way webpack handles ES modules.

For people who still want to keep the old behavior, they can explicitly add modules: true or e.g. modules: "amd".

Other information:

I am not happy at all with the implementation, but it works. Let me know if there is a better way I missed.

I used parts of the code in #485, so credits to @wardpeet!

Fixes #521, fixes #477, fixes #485

@Andarist
Copy link
Member

Andarist commented Nov 4, 2017

Override shouldnt be done with true but with a module type to match the mentioned presets options

@wardpeet
Copy link

wardpeet commented Nov 4, 2017

@SpaceK33z should I abandon my PR?

@SpaceK33z
Copy link
Author

SpaceK33z commented Nov 4, 2017

@wardpeet, yeah this would make your PR redundant. Sorry about that, but I did end up using much of your code!

@wardpeet
Copy link

wardpeet commented Nov 6, 2017

@SpaceK33z no worries, thought it did the same as my PR 👍

@danez
Copy link
Member

danez commented Nov 6, 2017

I would be still in favor of showing a warning similar to rollup-plugin-graphql instead of modifying the users configuration.
It always is a little bit frustrating to me when tools do this, because I guess that people who currently rely on babel producing commonjs (I also had to use this in production for a long time till I was finally able to turn it off) will have a hard time understanding why suddenly their correct config does not work anymore and does not do what they expect.

src/index.js Outdated
@@ -152,6 +152,34 @@ module.exports = function(source, inputSourceMap) {
options.sourceFileName = relative(options.sourceRoot, options.filename);
}

if (this.version > 1 && options.presets) {
const presetsWithModulesOption = ["es2015", "env", "latest"];
Copy link
Contributor

@michael-ciniawsky michael-ciniawsky Nov 6, 2017

Choose a reason for hiding this comment

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

const presets = [ "es2015", "env", "latest" ]

// not sure if really needed (?)
const name = (name) => name
  .replace(/^@babel\//, "")
  .replace(/^preset-/, "")

options.presets.forEach((preset) => presets.indexOf(name(preset[0])) > -1 
   ? preset[1] !== undefined 
     ? preset[1].modules = false
     : preset[1] = { modules: false }
   : preset
)

Copy link
Contributor

Choose a reason for hiding this comment

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

But I agree with @danez here that a warning with link(s) to the appropriate docs would be the saner solution. babel is configurable and modular by design and a dev simply needs to know how to configure it properly

@huchenme
Copy link

Guys, what is the status of this PR?

@SpaceK33z
Copy link
Author

I messed up this PR by using force push, so I made a new PR #650.

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.

Set modules: false by default on babel-preset-env Warning if transpiling ES modules to commonjs
6 participants