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

Error form UglifyJs #18

Closed
smuemd opened this issue Jul 3, 2018 · 24 comments
Closed

Error form UglifyJs #18

smuemd opened this issue Jul 3, 2018 · 24 comments

Comments

@smuemd
Copy link

smuemd commented Jul 3, 2018

Hello,

I am getting a strange error from UglifyJS.
Running this through webpack

Here is the error:

{ Error: 4.714f43602dc9cdd1454a.js from UglifyJs
Unexpected token: operator (>) [4.714f43602dc9cdd1454a.js:29,12]

It points to this section in the bss code:

const cssProperties = ['float'].concat(Object.keys(
  findWidth(document.documentElement.style)
).filter(p => p.indexOf('-') === -1 && p !== 'length'))

which looks fine.

I don't know what might cause this. The code works fine. It just wont minify.

@barneycarroll
Copy link
Contributor

UglifyJS can't handle ES6 IIRC

@porsager
Copy link
Owner

porsager commented Jul 3, 2018

That's right Barney.

@smuemd what bundler are you using? If rollup, then bss uses rollup-plugin-buble for it's browser bundle, so you could use that too to make things work. (you can look at the bss rollup.config.js too)

@smuemd
Copy link
Author

smuemd commented Jul 3, 2018

It's a webpack project with standard configuration via spike (https://github.com/static-dev/spike)

@barneycarroll It's actually using ES6 all over the place. bss is the only package where this issue crops up. Emotion works fine for example. But who would want to use that? ;)

@porsager
Copy link
Owner

porsager commented Jul 3, 2018

Oh I see..

I just tried a clean install of spike, and it doesn't complain using bss there.. Is the project one you can share?

Could you try to remove the node_modules folder package-lock.json and do npm install?
Googling for that specific uglify error also returns mostly 2017 results, so perhaps it's an old version causing this?

@smuemd
Copy link
Author

smuemd commented Jul 3, 2018

@porsager with spike the js minification only takes place in production mode:
you gotta run

$ npm run prod

I got another hint:

I just took the ./lib folder and droped it from node_modules/bss into my local project directory
then:

import b from './lib'

minification worked without errors. Does Rollup give the files some special treatment? I am not too familiar with the tool.

@porsager
Copy link
Owner

porsager commented Jul 3, 2018

Ah sorry, didn't think about that.. I'm reproducing here now too..

I think webpack/babel skips transpiling modules that has a module entry point field in its package.json 😑

If I delete the module field in node_modules/bss/package.json and set "main": "lib/index.js" it works fine... ugh..

@smuemd
Copy link
Author

smuemd commented Jul 3, 2018

Pointing main to bss.js and getting rid of the module declaration in package.json works too

what is the module declaration good for anyway?

@porsager
Copy link
Owner

porsager commented Jul 3, 2018

For bss not that much currently since it only has a single default export, but if it had others, bundlers would use the module entry point to allow for tree-shaking (remove unused code paths).

A side-effect which is nice is that you'll also get ES6 code directly instead of transpiled ES5 which makes sense in cases where you just want to run on ES6 supported clients.

There's some more on the module field here https://github.com/rollup/rollup/wiki/pkg.module

@porsager
Copy link
Owner

porsager commented Jul 3, 2018

I'm actually curious I can't find anything more about this anywhere.. I don't see why it would make sense for them "not" to transpile the module entry point...

@smuemd
Copy link
Author

smuemd commented Jul 3, 2018

interesting question indeed. I also tried theforceAllTransforms option for uglify-js. it just wont do it.

@smuemd smuemd closed this as completed Jul 3, 2018
@porsager
Copy link
Owner

porsager commented Jul 3, 2018

I think I'll keep this open until there's a resolution 😊 If you want to check in with webpack / spike, it's also good to refer here. I might also do that 😉

@porsager porsager reopened this Jul 3, 2018
@porsager
Copy link
Owner

porsager commented Jul 3, 2018

I think the issue lies in the transpilation step before uglify.js.

It's only uglify.es that supports ES6 (also outputs ES6)

@futurist
Copy link

futurist commented Jul 4, 2018

I'm facing the problem using webpack before, looks like below two line:

https://github.com/static-dev/spike-core/blob/a2743fb7197a780be5e63fc641b7bb25c7813dd8/lib/config.js#L117

https://github.com/static-dev/spike-core/blob/a2743fb7197a780be5e63fc641b7bb25c7813dd8/lib/config.js#L227

Cause babel-loader to exclude the node_modules folder, thus also exclude the installed bss module, so bss kept as ES6 module untouched, then lead to the uglify error.

For you own webpack config, solution is remove the exclude rule for babel-loader, but for spike, maybe you should dive into the API to find a way...

@porsager
Copy link
Owner

porsager commented Jul 4, 2018

Ah thanks a lot @futurist

The thing that confuses me is that if I remove the module field and point main to the ES6 entry point it actually does transpile it 🧐

@futurist
Copy link

futurist commented Jul 4, 2018

@porsager If you use babel-preset-env, then it's only support transpile below formats:

"amd" | "umd" | "systemjs" | "commonjs" | false, defaults to "commonjs"

https://babeljs.io/docs/en/babel-preset-env.html#modules

The source is here:

https://github.com/babel/babel/blob/afa1207224a500d358b36c9b9a6e7167bb386e3d/packages/babel-preset-env/src/module-transformations.js#L4

Then follow into transform-modules-commonjs, below line:

https://github.com/babel/babel/blob/afa1207224a500d358b36c9b9a6e7167bb386e3d/packages/babel-plugin-transform-modules-commonjs/src/index.js#L122

seems the plugin skip any isModule format, then going on to find isModule definition:

https://github.com/babel/babel/blob/afa1207224a500d358b36c9b9a6e7167bb386e3d/packages/babel-helper-module-imports/src/is-module.js#L12

I'm lost in path.node.sourceType === "module" line...

Looks like module never be touched by babel-preset-env?

Edit: I found some wrong thing for above, that "commonjs" format is for babel target output, so it's not related to this issue, since this issue is for input...

@smuemd
Copy link
Author

smuemd commented Jul 4, 2018

@futurist good find!
I opened an corresponding issue in the spike repo

@porsager
Copy link
Owner

@smuemd I looked at the spike issue, and I wasn't sure if my understanding that webpack 4 doesn't have this problem is correct? In that case I also think it would be fine to close this issue..

@futurist
Copy link

futurist commented Aug 2, 2018

I also faced the problem when using webpack@3, for spike-core below line caused this issue:

https://github.com/static-dev/spike-core/blob/ad4debf3009a5ce819f60ff48c9bc3c551e1d9a2/package.json#L31

Seems webpack 3 don't support module field very well...

@porsager
Copy link
Owner

porsager commented Aug 2, 2018

Thanks @futurist .. Do you have an easy workaround for a case like that? Maybe

import b from 'bss/bss'

I hope this is resolved with Webpack 4, but I don't have time to test it right now..

@futurist
Copy link

futurist commented Aug 2, 2018

Tested I have no issue again for webpack 3 when using import b from 'bss/bss', @smuemd could you have a test with spike?

Maybe can publish module with a version transpiled, without const etc, but have export default just for ES6?

@smuemd
Copy link
Author

smuemd commented Aug 2, 2018

@futurist It works! Uglify is not complaining anymore.
Alt Text
What is different now? What did I miss?

@porsager
Copy link
Owner

porsager commented Aug 2, 2018

Hehe...

When you're doing bss/bss you're importing the bundled & transpiled file ;)

@smuemd
Copy link
Author

smuemd commented Aug 2, 2018

Ha. I had no idea. Thought import b from 'bss would actually do that as specified in package.json main

@porsager
Copy link
Owner

porsager commented Aug 2, 2018

Yeah that's what you would think, but webpack chooses to pick the module field, and then not include it in it's transpilation pipeline 😔

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

No branches or pull requests

4 participants