google / closure-compiler-js

Package for the JS version of closure-compiler for use via NPM
https://github.com/google/closure-compiler
Apache License 2.0
1.84k stars 64 forks source link

Use webpack plugin API instead of console to report errors and warnings #70

Closed dkastenholz closed 7 years ago

dkastenholz commented 7 years ago

The closure-compiler-js webpack plugin is currently using the globally defined logger for error reporting, which ultimately dumps all errors to stdout.

This is not how errors should be handled in webpack. Admittedly, the webpack docs don't give too much information, but it seems to be common practice to push errors into the compilation.errors array, so that webpack can later handle (e.g. format) all errors equally.

At the moment, it is difficult to capture closure-compiler-js errors in webpack builds. Build environments that follow the usual convention will not capture the error details at all.

A possible workaround is to override the global logger property, but IMO that is a rather dirty hack, and since the compilation object is not exposed to the logger function, the new handler could possibly suppress the stdout logging, but not redirect the errors properly.

See here for a discussion: https://stackoverflow.com/questions/42364742/webpack-plugin-error-management

googlebot commented 7 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


dkastenholz commented 7 years ago

Hello @blickly / @samthor, what do you think about this? Please let me know if you need anything changed. (Maybe options to toggle stdout and webpack API logging, to make the change non-breaking?) Thanks!

googlebot commented 7 years ago

CLAs look good, thanks!

samthor commented 7 years ago

I'm not a webpack expert but I believe that this looks good.

samthor commented 7 years ago

Thanks!

dkastenholz commented 7 years ago

@blickly, would you mind triggering a deployment on npmjs.com? Thanks

blickly commented 7 years ago

I think that @MatrixFrog is in the middle of a release right now, although I'm not sure if that will include this change or it will come in the next one.

MatrixFrog commented 7 years ago

Just published 20170626.0.0 which should include this change, yes.