search

photostructure / batch-cluster.js

Parallelized and efficient Node.js support for batch-mode child processes
https://photostructure.github.io/batch-cluster.js/
MIT License
14 stars 3 forks source link

Batch-Cluster rejects Promises with a non-Error #3

Closed nknapp closed 6 years ago

nknapp commented 6 years ago

I noticed the following error message when using exiftool-vendored in my mocha-tests:

Error: the string "stderr.data: Warning: No writable tags set from tmp/ffmpeg/lumix/PRIVATE/AVCHD/BDMV/STREAM/00000.MTS" was thrown, throw an Error :)

I think, the reject function of a promise should always be called with an Error, not a string. One particular reason is, that stack-traces cannot be transported on a string.

This line in BatchProcess.ts calls the "onError"-function with a string as parameter. This line in Task.ts calls the "reject" function of the deferred with this string, which ultimately leads to the Promise being rejected with a string as "reason". I think this should be changed, although I can see this is a breaking change.

I'd be happy to create a PR, if you like.

mceachen commented 6 years ago

Hey, sorry for the delay, this didn't show in my github notifications! You are right--I think I was concerned about the weight of Error creation, but that's certainly premature optimization in retrospect. I'll update now.

mceachen commented 6 years ago

Released both v3.0.0 batch-cluster and v5.0.0 of exiftool-vendored. Thanks again for the idea.