Handle errors with promise rejections #695
Conversation
|
I've tested this by updating our application to use a pptxgenjs distribution compiled from this PR, and seeing that we now correctly catch the errors caused by undefined |
| }) | ||
| return this.exportPresentation(outputType) | ||
| .catch(ex => { | ||
| throw new Error(ex.message + '\nDid you mean to use writeFile() instead?') |
There was a problem hiding this comment.
FWIW: This kills the stack trace of the actual exception, which seriously sucks for debugging. I would suggest to actually drop this as well, and just do:
write(outputType: JSZIP_OUTPUT_TYPE): Promise<string | ArrayBuffer | Blob | Buffer | Uint8Array> {
return this.exportPresentation(outputType)
}|
@gitbrent do you maybe have some time to look over this? Right now the PptxGenJS library kills our processes due to the unhandled rejections, and this patch seems to fix things nicely -- but of course we don't want to run with local patches if possible :) |
|
Anything I can do to help move this forward? |
The code used promises, but in most cases dropped errors and rejects "on the floor", rather than just exposing them further to the caller. This made it impossible to actually catch these problems, leading to NodeJS warnings about "unhandled promise rejections" (which will soon make the NodeJS process crash!)
|
Rebased onto current master to resolve the conflict introduced through #694 |
|
Thanks @ankon - great work! Appreciate the Promise expertise. |
The code used promises, but in most cases dropped errors and rejects "on the floor", rather than just
exposing them further to the caller. This made it impossible to actually catch these problems, leading
to NodeJS warnings about "unhandled promise rejections" (which will soon make the NodeJS process
crash!)
This is related to #678, and at least allows to catch the errors.
The fixes follow a couple of patterns:
Remove useless wrapping promises when there is already one available
This can be written as simply
return doSomePromiseCode, which also makes the caller able to handle the rejection.Move the use of
new Promise()calls for callbacks as close to the callback as possible.Make sure to return promises (for example the result of
Promise.all)Note that this does still contain the
Presenationtypo inwriteFile, see #694 for that.