Warning if transpiling ES modules to commonjs

[jakearchibald]

Some presets, such as 'es2015' will transpile ES6 modules to commonjs. This works fine, but it prevents webpack performing treeshaking, meaning bundles could be unnecessarily larger.

Is there any reason users would want to transpile to commonjs at this point in the pipeline? If not, babel-loader could attempt to change the default here, or at least display a warning if it detects this kind of transpilation occurring.

[elrumordelaluz]

@jakearchibald based on the babel-preset-es2015 documentation you could set something like:

options: {  
  presets: [ 
    [ 'es2015', { modules: false } ] 
  ]         
}

Setting this to false will not transform modules

update: understood that the point is making it as a default behaviour, agree!

[jakearchibald]

Yeah, I used the same workaround, but since babel-loader is built for webpack, and webpack handles modules better than commonjs (eg tree shaking), it should be the default.

Unless there's a reason users would want to transpile to commonjs before passing to webpack?

[Couto]

Imho, this would break the predictability of both tools.

Webpack and Babel are already hard to set up, and messing with the tools' defaults will probably cause a lot of unaware mistakes. Babel recommends to turn the option off when you want to delegate the modules' resolution to another tool. I think we should keep the option explicit, instead of having the tools to mess with the options behind the scenes.

I do believe, however, that we could update the README.md examples to include that option, since like you stated, it does make sense to delegate the modules resolution to Webpack.

[jakearchibald]

@Couto

this would break the predictability of both tools … messing with the tools' defaults will probably cause a lot of unaware mistakes

Right, but you have to balance that with the unaware mistake of losing tree shaking without warning.

Sure, you could add something to the readme along the lines of "btw a lot of the common defaults do the wrong thing", but a runtime warning sounds a lot more useful.

[Couto]

If we don't mess with the option behind the scenes, and it just throws a warning, I think that's a cool feature.

Would you be willing to do a PR?

[jakearchibald]

I'm really just learning webpack right now & creating issues as I find them as a user. I don't know how plugins are made (nor I'm I familiar with the internals of Babel), so it might be a while before I can ramp up to a decent PR. Eg, I'm sure there's a standard way to do logging, but I have no idea.

But if no one knowledgeable is willing to take it, I guess I could get to it eventually.

[TheLarkInn]

At the least, I think that providing a warning would be wonderful.

@jakearchibald webpack supports more than one target. So we have to be as open as possible so that those who transpile esm to cjs for their node webpack bundles, can do so by default. However a warning is incredible beneficial.

[jakearchibald]

@TheLarkInn I guess the warning could only display for web targets? Maybe that isn't available to babel-loader. Anyway, I'll need to do isomorphic stuff for the thing I'm playing with, so I might be able to bring an informed opinion next week 😄

[TheLarkInn]

Yes totally!! Plz do. I mean, in a perfect world, loaders shouldn't directly access config options, but I believe they can (in some cases).

[wardpeet]

@Couto @jakearchibald I could give it a spin as I believe this would be great.

[Couto]

That's great! Ping if you have any questions I'm sure we all be happy to help! :D

…

On June 23, 2017 6:48:40 PM GMT+02:00, Ward Peeters ***@***.***> wrote: @Couto @jakearchibald I could give it a spin as I believe this would be great. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #477 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[wardpeet]

will start on this tomorrow 👍

[woutervanvliet]

When working with codebases that both use modules and commonjs, modules: false would cause problems.

Good enough reason for me to not make this a default in babel-loader.

See fx: webpack/webpack#4039

[wardpeet]

@woutervanvliet it will just be a warning and I guess most of the users will not have multiple types of imports

[woutervanvliet]

@wardpeet Agree - not many, but some. Would imagine in particular some who might be in transition from the one to the other. Found out about it because I ran into this issue.

Would be great if there was an easy, obvious way to suppress the warning. For example by specifying an explicit other option than false for the modules option.

[TheLarkInn]

Yes we should be doing this. I will do the PR myself if I have to.

There is no sensible reason to downgrade to cjs from esm if using webpack 2+

[wardpeet]

PR is still open.. You can always change things on my PR.

[loganfsmyth]

@TheLarkInn My primary concern is that there's no guarantee that they will behave identically. Either Babel or Webpack could have bugs in their implementation. We've generally shied away from having build tools change behavior that hasn't been encoded into the user's config by the user themselves.

[SpaceK33z]

PR #529 should fix this; instead of giving a warning it modifies the config to set modules: false on the presets env/latest/es2015. It has the downside of being a bit of magic since it silently modifies your Babel config. But for most people this means less configuration and as a result, one less configuration option to understand.

[loganfsmyth]

This should be resolved with #660 in Babel 7.x