Missing pdftk executable causes promise chain to hang

[bsdkurt]

Throwing in the ChildProcess error event causes the output() promise to never resolve. Call reject() with the error instead so that a missing pdftk executable doesn't hang the promise chain.

[jjwilly16]

Good catch...no pun intended. Thank you! I'll merge and push out a new release to npm before tomorrow.

[bsdkurt]

Your welcome! Thank you for creating node-pdftk, it fits my needs nicely.

I'd like to make a suggestion. When looking at this I noticed other methods can throw before output() is called, technically this is fine and works when the call to pdftk is in a try/catch block or called within a .then(), but is somewhat unexpected for a promise returning api. For example in Express example - render directly in browser if the input() method throws, next() will not be called.

One idea to eliminate the throws would be to not-throw and cache the error in the class, then all methods would check at the beginning if error is set, then return this. output() would check at the begging too but reject(this.error). To be super safe the pre-promise methods would all follow this pattern:

method(args) {
  if (this.error)
    return this;
  try {
    <existing logic>
  }
  catch (e) {
    this.error = e;
  }
  return this;
}

With this pattern, all errors would be forwarded to the promise chain and your api now could be used in the express example without issues. (input() method would be slightly different but you get the idea).

Thoughts?

[jjwilly16]

To be honest, I did a lot of learning while writing this library. Looking at it now, I completely understand how the throws can be problematic.

When I find some time, I will definitely look into cleaning them up and I'll probably borrow your suggestion for caching the errors and funneling them through the promise - I like that idea.

Thanks for your suggestion and if you want to take a shot at implementing it yourself, I always accept pull requests!

[jjwilly16]

@bsdkurt - I was looking at the code and realized that all of the throws I had in place were just unnecessary. Pdftk handles these errors just fine. I removed them in my latest commit - these errors will now be passed through the promise chain. Thanks again for your input.

https://github.com/jjwilly16/node-pdftk/commit/2db0d5054860f3f9329eace5a59ff6c08215979d