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

Handle incompatibility with Webpack #39

Open
eliihen opened this issue Dec 22, 2016 · 5 comments
Open

Handle incompatibility with Webpack #39

eliihen opened this issue Dec 22, 2016 · 5 comments

Comments

@eliihen
Copy link

eliihen commented Dec 22, 2016

Hi,

First of all, thanks for a pretty cool plugin.

I must admit I am a little disappointed to see that this plugin was the cause of my two day grief of trying to get Webpack 2 tree-shaking to work. You see, it is incompatible with Webpack 2 when it uses native ES6 modules. In fact, it breaks the entire website when you try to use it. Have a look at this StackOverflow post I created for posterity for details. I also created a tiny reproduction repository you can find here if you want to see it for yourself.

If possible, I would like to make a suggestion for a feature that can save other people the pain I went through. Is it possible for you detect the usage of Webpack 2 with native ES6 modules somehow and handle this better?

@lijunle
Copy link
Contributor

lijunle commented Dec 23, 2016

Hi, @esphen This is an interesting bug.

With webpack output, why it use a instead of default for default export?

/* harmony default export */ exports["a"] = _default; // exports = Object {}

@eliihen
Copy link
Author

eliihen commented Dec 23, 2016

With webpack output, why it use a instead of default for default export?

I honestly don't know. If you want an answer to that you probably need to get in touch with someone who works on webpack.

I'm not a webpack developer, so I don't have the definitive answers, but I had a look at the Webpack source to try to help. On the face of things, assigning default to exports["a"] it seems to be a deliberate decision. Look at this documentation, where they present the resulting compilation as /* harmony export (immutable) */ exports["a"] = increment;. Since it is documented as an example, I think we can more or less rule out it being a bug in webpack.

That's how it becomes exports["a"]. It seems to be an implementation detail on how they handle ES6 modules.

@lijunle
Copy link
Contributor

lijunle commented Dec 23, 2016

More strange here. In the webpack example, the increment function is not even the default export! I am guessing webpack use a, b, c, etc. to be the export name.

My personal feeling is, webpack@2 is awesome to natively support ES6 import syntax. We should follow the right ES6 import syntax to make webpack@2 happy. In such case, my suggestion is, this plugin detects it is currently running in webpack@2 environment, report an error and that is all.

@59naga You ideas?

@eliihen
Copy link
Author

eliihen commented Dec 23, 2016

In the webpack example, the increment function is not even the default export!

Shoot, my bad. I missed that!

my suggestion is, this plugin detects it is currently running in webpack@2 environment, report an error and that is all.

Be aware that this is only an issue if you disable babel's module compilation with "presets": [["es2016", {"modules": false}]] (recommended with webpack 2). Thus only checking for webpack@2 and throwing would make it so that people who do not set {"modules": false} can not use this plugin anymore.

But on a general basis I totally agree that giving some sort of message why it is breaking is good. But can we do it without breaking it for people who for whatever reason can not, or do not want to use webpack@2 with native ES6 modules?

DanReyLop pushed a commit to Automattic/woocommerce-services that referenced this issue Feb 20, 2017
The Babel option "modules: false" can't be used with the add-module-exports, which is needed for Calypso to compile.
This means we can't take advantage of WebPack2 tree-shaking yet, thus adding 50KB to the JS bundle.
More info:
59naga/babel-plugin-add-module-exports#39 (comment)
Automattic/wp-calypso#10799
Automattic/wp-calypso#11204
@linjiyeah
Copy link

I met this problem too...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants