Skip to content

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

Closed
SpaceK33z wants to merge 0 commit intobabel:masterfrom
SpaceK33z:disable-modules-default
Closed

Add modules: false by default for es2015/env/latest presets#529
SpaceK33z wants to merge 0 commit intobabel:masterfrom
SpaceK33z:disable-modules-default

Conversation

@SpaceK33z
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

wardpeet commented Nov 4, 2017

@SpaceK33z should I abandon my PR?

@SpaceK33z
Copy link
Copy Markdown
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
Copy Markdown

wardpeet commented Nov 6, 2017

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

@danez
Copy link
Copy Markdown
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
}

if (this.version > 1 && options.presets) {
const presetsWithModulesOption = ["es2015", "env", "latest"];
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

Guys, what is the status of this PR?

@SpaceK33z
Copy link
Copy Markdown
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