search

roman01la / webpack-closure-compiler

[DEPRECATED] Google Closure Compiler plugin for Webpack
MIT License
464 stars 25 forks source link

Add support for sourcemaps and limit total concurrency #5

Closed jscheid closed 8 years ago

jscheid commented 8 years ago

Hi @roman01la, this isn't particularly tidy but I believe it's an improvement over master, so I thought I'd send this over. The changes should be self-explanatory but let me know if you have any questions or would like to discuss more.

roman01la commented 8 years ago

Hi. Thanks, source maps are definitely a good thing. Why did you removed default options? Also compilation will fail if there's no options passed in you need to check it before accessing them.

jscheid commented 8 years ago

Why did you add them? Google Closure Compiler already has default options. It seems confusing that a user will be faced with different default options when going through this tool, compared to invoking the vanilla compiler.

I'll have a look at the case of no options being passed in, thanks!

jscheid commented 8 years ago

@roman01la I've fixed the issue you've found when no options are passed in.

jscheid commented 8 years ago

Hi @roman01la, what do you think of the code now that I've fixed the case of no options passed in?

Are the default options a deal breaker for you? I still think it's a good idea to just use the upstream defaults (principle of least astonishment and all that) but I'm happy to move that change into a separate PR so we can move ahead here. Any thoughts?

jscheid commented 8 years ago

By the way we've been using this branch in our continuous deployment pipeline for the last couple of months and there have been no issues, so I think it's pretty reliable.

roman01la commented 8 years ago

Hi @jscheid. Sorry I didn't have a chance to check this. I'm going to accept PR, thank you!

roman01la commented 8 years ago

I'm also very surprised you are actually using it! Is there any chance you could tell me what's your company name? :)

roman01la commented 8 years ago

Can you please also fix the tests? I've just tried to run them locally and it's failing.