jakejs / jake

JavaScript build tool, similar to Make or Rake. Built to work with Node.js.
http://jakejs.com
Apache License 2.0
1.97k stars 190 forks source link

Improved exception handling when a child process fails #373

Open pmarrapese opened 4 years ago

pmarrapese commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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. :)