Skip to content
This repository has been archived by the owner on Dec 5, 2019. It is now read-only.

Support UglifyJS 3 #32

Closed
bebraw opened this issue May 10, 2017 · 25 comments
Closed

Support UglifyJS 3 #32

bebraw opened this issue May 10, 2017 · 25 comments

Comments

@bebraw
Copy link
Contributor

bebraw commented May 10, 2017

Currently using UglifyJS 3 breaks the plugin. It has a new API so the plugin has to change accordingly.

@rainjay
Copy link

rainjay commented May 10, 2017

if u are using yarn, paste the codes below in yarn.lock

"uglify-js@git://github.com/mishoo/UglifyJS2#harmony":
  version "3.0.0"
  resolved "git://github.com/mishoo/UglifyJS2#3fac29a01787b07ce9e43fb87a17bde33123509d"
  dependencies:
    commander "~2.9.0"
    source-map "~0.5.1"
  optionalDependencies:
    uglify-to-browserify "~1.0.0"

Replace the old one, yarn and then it will work, Although it is not recommended to edit in yarn.lock.
Just a temporary solution.

@t47io
Copy link

t47io commented May 11, 2017

I see this error:

Error: Cannot find module 'uglify-js'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Website_Server/t47io/main/node_modules/uglifyjs-webpack-plugin/dist/index.js:19:14)
    at Module._compile (module.js:571:32)
    at Module._extensions..js (module.js:580:10)
    at Object.require.extensions.(anonymous function) [as .js] (/Website_Server/t47io/main/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Website_Server/t47io/main/build/plugins.js:1:1)
    at Module._compile (module.js:571:32)
    at loader (/Website_Server/t47io/main/node_modules/babel-register/lib/node.js:144:5)

Need to rename to 'uglify-es'?

@bebraw
Copy link
Contributor Author

bebraw commented May 11, 2017

uglify-es won't work with the current version of the plugin without serious changes.

@rainjay
Copy link

rainjay commented May 11, 2017

another way: paste "uglify-js": "git://github.com/mishoo/UglifyJS2#harmony-v2.8.22" in your devDependency in package.json

@ro-savage
Copy link

@bebraw - Is this something that is currently being worked on?

I had a look at the source to see if I could do a PR to support UglifyJS3 but it looks like this is using a lot of undocumented and internal APIs of UglifyJS2 and felt like it would be almost a total rewrite.

Maybe @alexlamsl or @kcz could help?

@bebraw
Copy link
Contributor Author

bebraw commented May 24, 2017

@ro-savage I don't think anyone is working on this at the moment as people are busy with other things. It's a big rewrite, yeah.

syuilo added a commit to misskey-dev/misskey that referenced this issue May 24, 2017
@noBlubb
Copy link

noBlubb commented May 24, 2017

I created an updated fork two days ago, that has been forked and improved again here https://github.com/Nckcol/uglify-es-webpack-plugin and is now almost feature complete.

@bebraw
Copy link
Contributor Author

bebraw commented May 24, 2017

@noBlubb Do you want to look into merging the work back here or do you prefer to maintain it on your own? Webpack main repository has some Uglify tests that have to be ported over and adjusted.

@noBlubb
Copy link

noBlubb commented May 24, 2017

I would like to transfer the plugin back to webpack-contrib, as I do not feel ready to maintain this all by myself. This started of as some kind of POC for me and I'm happy to give something back to the webpack community :)

@bebraw
Copy link
Contributor Author

bebraw commented May 24, 2017

@noBlubb Ok. I guess the question is whether to maintain it as a package of its own or to merge here somehow. I think the overall direction is that we want to get rid of the webpack core version entirely (too coupled).

@d3viant0ne Any thoughts on this?

@ro-savage
Copy link

@nckcol your thoughts?

I am happy to help out where I can as well.

The uglify-es-webpack-plugin seems to be using only (mainly?) external APIs which makes it easier to get involved with.

@dlebedynskyi
Copy link

dlebedynskyi commented May 24, 2017

@noBlubb tested your project. Had one issue with Extract Text Plugin.
Going to write here since may be merged soon.
With following set up

const UglifyPlugin = require('uglify-es-webpack-plugin');
// ...
const plugins = [
  // extract styles as single file
  new ExtractTextPlugin({ filename: '[name].[contenthash].css', allChunks: true }),
  // min
  new UglifyPlugin(),
  // assets

Error

ERROR in UglifyEsPlugin - app.7fcd3e13c555ad6252ad521f8cc10d8c.css
{"message":"Unexpected token: keyword (var)","filename":"app.7fcd3e13c555ad6252ad521f8cc10d8c.css","line":1,"col":243,"pos":243}

Had tried

new UglifyPlugin({exclude: /\.css$/ }),

Same result.

@noBlubb
Copy link

noBlubb commented May 24, 2017

@ro-savage Absolutely! The forked version is as minimal as possible right now.

@dlebedynskyi Options are not yet supported, you might want to try @nckcol's fork.

@nckcol
Copy link

nckcol commented May 24, 2017

Hey! I'm working on friendly error and warning output now.
I don't know how to make cheap maps work with uglify-es. Any ideas?

@dlebedynskyi I fixed this in my fork

@noBlubb
Copy link

noBlubb commented May 25, 2017

@nckcol #22
Seems like this has not been supported by the old plugin.

@nckcol
Copy link

nckcol commented May 27, 2017

@bebraw
I have already done all plugin features that was in old version
Do you interested in merging it back?

@bebraw
Copy link
Contributor Author

bebraw commented May 27, 2017

@nckcol Are you interested in maintaining the work over here?

@nckcol
Copy link

nckcol commented May 27, 2017

@bebraw Yes, but I'm afraid I will rarely be able to maintain it
I'll try to do everything i can to make it easier to maintain.
What do you think about it?

@bebraw
Copy link
Contributor Author

bebraw commented May 27, 2017

@nckcol No probs, here's a rough roadmap:

  1. Upgrade the project to webpack-defaults. I can do that (~30 mins of work). This gives a solid testing infrastructure etc.
  2. Cut a 1.0 release for the plugin. This is going to be the last release that supports Uglify 2.
  3. Merge your PR so we get Uglify 3 support. Important: we have to use optionalDependency here and mark both Uglify and uglify-es as one so the user can choose. The plugin should handle this with a conditional import (check for one before another).
  4. Release 2.0 of the plugin with Uglify 3 support (we can jump to 3 if that sounds better, the versions don't have to be in sync).

There's also webpack core side to worry about as now it comes with a Uglify plugin of its own. I guess we want to drop that in webpack 3 entirely and point people to this plugin instead.

Note that we can compensate for maintenance efforts up to a point. I can also invite you to our contrib group (send me mail) so it's easier to discuss the next moves.

@nckcol
Copy link

nckcol commented May 27, 2017

Well, I finished the first item
@bebraw Could you explain in more detail what actions are required of me to complete the second item on the list?

@bebraw
Copy link
Contributor Author

bebraw commented May 27, 2017

@nckcol We have to get #35 merged first (needs some reviews before it can go in). I sent you an invite so it's easier to coordinate this work. Thanks. 👍

@lukeapage
Copy link
Contributor

Merge your PR so we get Uglify 3 support. Important: we have to use optionalDependency here and mark both Uglify and uglify-es as one so the user can choose. The plugin should handle this with a conditional import (check for one before another).

Imo you should do an option. In a typical dev setup, uglify is a dependency of a few modules - and if more start using uglify-es then it will be impossible for the consumer to tell this plugin what to use.

@bebraw
Copy link
Contributor Author

bebraw commented Jun 7, 2017

Imo you should do an option. In a typical dev setup, uglify is a dependency of a few modules - and if more start using uglify-es then it will be impossible for the consumer to tell this plugin what to use.

Yeah, we have to keep it flexible so the user can choose between vanilla and ES. It gets more interesting if you want both for some reason, though (can this happen?). Maybe this means the plugin should be able to accept uglify as an option too?

@nckcol Please join the Slack when you have time so we can coordinate this work.

@mlazowik
Copy link
Contributor

Related: webpack/webpack#5028

@michael-ciniawsky
Copy link
Member

Closed by #63

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

No branches or pull requests