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

Ensure error is logged when Closure fails #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ian-craig
Copy link
Contributor

Fixes #162.

Whenever the compiler process exits with non-zero code, we should be reporting some error back to Webpack.

It looks like the on close for compilerProcess has some custom error (and warning?) handling around parsing stderr, but simply calls reject() with no error at the end. The result is for non-zero exit but no stderr we reject, but never pass an error to Webpack.

I think most cases could be cleaned up by passing the error through reject(error) instead of this.reportErrors(compilation, [error]), but not if the stdout parsing logic found warnings but no errors. Instead I've opted to just add another case here that's handled, but I'm not 100% sure there are no other edge cases here.

@ChadKillingsworth
Copy link
Member

What's a good way to trigger this just so I can double check it does what it says?

@ian-craig
Copy link
Contributor Author

The real scenario I hit was a missing dependency which caused the windows version of Closure to fail to run. I suspect that the error was in Windows or whatever wrapper Closure uses to compile a "native" version from Java, so there wasn't the usual error handling from Closure itself.

As a simple test I override the java exe path and ran with platform ['java'].

ClosureCompiler.prototype.javaPath = "my/script/which/exits/1";
new ClosurePlugin({ platform: ['java'] }, ...);

If you do this before applying this PR fix you should see that Webpack passes with no errors, but the bundle is unminified.

@ChadKillingsworth
Copy link
Member

Retriggering CI

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@aboktor
Copy link

aboktor commented Sep 23, 2022

@ian-craig I believe this requires you to sign the CLA

@ian-craig
Copy link
Contributor Author

Signed, hopefully you're good to go now 🚀
Thanks for the bump @aboktor

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

Successfully merging this pull request may close these issues.

When Closure exits with non-zero code but no stderr, output is not minified and there are no errors
3 participants