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

Improved exception handling when a child process fails #373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pmarrapese
Copy link

Presently, if Jake fails due to a child process failing, the exception is very confusing. This is especially the case on Windows, where the "command not found" message takes up 2 lines.

This pull request changes how Exec handles errors by using a proper Error object instead of merely concatenating the output of stderr. As a result, api.fail will not attempt to build the Error object from scratch and potentially mutilate the child process's error message.

Example (without patch):

[paul@tempest:myproject]$ jake
jake aborted.
operable program or batch file.
(See full trace by running task with --trace)
[paul@tempest:myproject]$ jake --trace
jake aborted.
operable program or batch file.

Example (with patch):

[paul@tempest:myproject]$ jake
Starting 'lint'...
jake aborted.
Error: 'jshint' is not recognized as an internal or external command,
operable program or batch file.
    at Socket.handleStderrData (C:\Users\Paul\src\jake\lib\utils\index.js:169:29)
    at Socket.emit (events.js:198:13)
    at addChunk (_stream_readable.js:288:12)
    at readableAddChunk (_stream_readable.js:269:11)
    at Socket.Readable.push (_stream_readable.js:224:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:94:17)

Now, it is much more clear why the child process failed.

@mde
Copy link
Contributor

mde commented Apr 6, 2020

I can see where this would be an inconvenience — I'm curious though, why are you not simply using the native execSync everywhere? All the Exec code in Jake pre-dated the existing of a synchronous exec, and was only there to make shelling out asynchronously a little less painful.

@pmarrapese
Copy link
Author

In this case, Leaflet.markercluster was still using the jake.exec method and throwing a very confusing error message because I didn't have jshint installed. Only took a few moments to figure out what was going on, so figured I'd share the knowledge. :)

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.

2 participants