feat: Added warning when modules is not false (#477) by wardpeet · Pull Request #485 · babel/babel-loader

wardpeet · 2017-06-27T22:15:55Z

Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting

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, ...)

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

What is the new behavior?
When webpack is version 2 or higher and we're using es2015 or env preset
We throw a warning saying that webpack can't treeshake correctly

Does this PR introduce a breaking change?

Yes
No

If this PR contains a breaking change, please describe the following...

Impact: Users with not having the right configs on webpack2 will get a warning
Migration path for existing applications:
Github Issue(s) this is regarding: Warning if transpiling ES modules to commonjs #477

Other information:

When webpack is version 2 or higher and we're using es2015 or env preset We throw a warning saying that webpack can't treeshake correctly Closes babel#477

wardpeet · 2017-06-27T22:16:54Z

it's not done yet. I have to add a check so it only fires the warning once and add some more tests to make sure it works 100%.

Note that I only check modules on es2015 and env preset.

existentialism · 2017-06-27T22:20:55Z

src/index.js

 const fs = require("fs");

+const presetsToCheckForModules = ["es2015", "env"];
+const modulesWarningMsg = `We've noticted the option modules isn't set to false,


Nit: noticted -> noticed

thanks not sure this is the message we want to show though 😛 Just wrote something quick.

michael-ciniawsky · 2017-06-27T22:36:46Z

src/index.js

+          (opts && !opts.hasOwnProperty("modules")) ||
+          (opts && opts.modules))
+      ) {
+        this.emitWarning(new Error(modulesWarningMsg));


this.emitWarning(` ⚠️ Babel Loader\n We've noticed that module transpilation (`option.modules`) is enabled. webpack supports ES2015 Modules natively and module transpilation disables tree shaking. It is recommended to disable it (https://github.com/babel/path/to/explanation) `)

The message can of course be improved in terms of spelling, but the message convention should be like above :)

great thanks!

michael-ciniawsky · 2017-06-27T22:38:20Z

src/index.js

+      }
+
+      if (
+        presetsToCheckForModules.indexOf(name) > -1 &&


Just add the {Array} here :)

michael-ciniawsky · 2017-06-27T22:47:04Z

src/index.js

 const pkg = require("../package.json");
 const fs = require("fs");

+const presetsToCheckForModules = ["es2015", "env"];


Are these two really to only ones ?

we also could add latest although I think it is deprecated.

I don't know how many there are to be honest..

nathanhammond · 2017-06-28T13:31:51Z

Notes from the meeting:

Your goal is to allow the syntax to parse, but enable Webpack to do the transformation.

We don't want to tie this to the preset string, instead we want to ensure that Babel knows that the babel syntax plugin is enabled and the transform plugin is not being run.

That doesn't mean that we won't accept this in present state to alleviate the worst problems, just that we have a target for the future.

The issue is that if you make your own preset it won't provide any warning to the user. This PR at present covers the 95% use case for minimal effort which is a significantly positive improvement.

wardpeet · 2017-06-28T21:22:12Z

@nathanhammond perhaps there is a way to see if babel has ran the module transformation?

danez · 2017-06-29T04:08:19Z

src/index.js

+        this.emitWarning(
+          new Error(
+            `\n\n⚠️  Babel Loader\n
+It looks like your Babel configuration specifies a module transformer. Please disable it.


Can we maybe give a little bit more information? Like "... . Please disable it, in order to allow webpack to create optimized bundles." Or "... . This disables tree shaking in webpack and will produce larger bundles. Please disable it."

Did you maybe forget to push? :)

yes sorry :)

danez · 2017-07-22T10:46:24Z

src/index.js

+            (opts && !opts.hasOwnProperty("modules")) ||
+            (opts && opts.modules)))
+      ) {
+        emitCache.set(options.presets, true);


should this be name instead of options.presets? Otherwise can you explain why you are setting the array as key?

The reason why I chose options.presets over name is that I cache a whole collection instead of just 1 preset.

Let me explain a bit more clearly. Lets say you have different load rules which use a different set of rules.

{ test: /\.js$/, exclude: /(node_modules|bower_components)/, use: { loader: 'babel-loader', options: { presets: ['es2015'], plugins: [require('babel-plugin-transform-object-rest-spread')] } } }, { test: require.resolve('legacy-module'), use: { loader: 'babel-loader', options: { presets: ['es2015'], } } }

Now I get 2 warnings because I've setup 2 load rules instead of just 1 as the presets are different

loganfsmyth

We were just talking about this issue again today and I figured I'd look over this PR. Not sure if you're even still interested in working on it since it's a bit stale now, but wanted to give my feedback. We can always take it from here too if you'd like.

loganfsmyth · 2017-10-30T21:49:53Z

src/index.js

    options.sourceFileName = relative(options.sourceRoot, options.filename);
  }

+  if (this.version > 1 && options.presets) {


Can we do Array.isArray(options.presets), so if the user passes an invalid valid, it'll pass through to babel-core to validate?

Also seems like we could check && !emitsCache.has(options.presets) here instead?

loganfsmyth · 2017-10-30T21:51:16Z

src/index.js

 const fs = require("fs");

+// we keep track of each preset config
+const emitCache = new Map();


Seems like this should be a Set rather than Map, and should ideally be a WeakSet?

loganfsmyth · 2017-10-30T21:55:11Z

src/index.js

+  if (this.version > 1 && options.presets) {
+    options.presets.forEach(preset => {
+      let name = preset;
+      let opts = {};


Seems easier to default to undefined since the object requires more steps to validate.

loganfsmyth · 2017-10-30T22:44:11Z

src/index.js

+        ["es2015", "env", "latest"].indexOf(name) > -1 &&
+        (!emitCache.has(options.presets) &&
+          (!opts ||
+            (opts && !opts.hasOwnProperty("modules")) ||


I'd avoid opts.hasOwnProperty as an instance property. Also you never validate that opts is an object, so this would throw if someone passed an invalid config instead of passing through to babel-core's validation. I'd go with

const PRESET_WHITELIST = new Set(["es2015", "env", "latest"]); // ... const { modules } = typeof opts === "object" && opts || {}; if (PRESET_WHITELIST.has(name) && modules !== false) { this.emitWarning // ... }

wardpeet · 2017-11-06T15:44:26Z

closing in favor of #529

feat: Added warning when modules is not false

cdb8bf2

When webpack is version 2 or higher and we're using es2015 or env preset We throw a warning saying that webpack can't treeshake correctly Closes babel#477

wardpeet force-pushed the feature/add-warning-withmodules branch from 3d15909 to cdb8bf2 Compare June 27, 2017 22:16

existentialism reviewed Jun 27, 2017

View reviewed changes

michael-ciniawsky suggested changes Jun 27, 2017

View reviewed changes

Review changes to update tests & messages

54821c2

danez reviewed Jun 29, 2017

View reviewed changes

Update warning message

0367b4b

danez reviewed Jul 22, 2017

View reviewed changes

hzoo mentioned this pull request Oct 27, 2017

Set modules: false by default on babel-preset-env #521

Closed

loganfsmyth reviewed Oct 30, 2017

View reviewed changes

SpaceK33z mentioned this pull request Nov 4, 2017

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

Closed

11 tasks

wardpeet closed this Nov 6, 2017

SpaceK33z mentioned this pull request Aug 7, 2018

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

Closed

11 tasks

Uh oh!

Conversation

wardpeet commented Jun 27, 2017

Uh oh!

wardpeet commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wardpeet commented Jun 28, 2017

Uh oh!

danez Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganfsmyth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet commented Nov 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wardpeet Jun 28, 2017 •

edited

Loading

michael-ciniawsky Jun 27, 2017 •

edited

Loading

nathanhammond commented Jun 28, 2017 •

edited

Loading

danez Jun 29, 2017 •

edited

Loading