Uncaught exceptions aren't catched or rejected #111
Conversation
js/load-image-scale.js
Outdated
| } else if (options.imageSmoothingQuality) { | ||
| ctx.imageSmoothingQuality = options.imageSmoothingQuality | ||
| } | ||
| ctx.drawImage( |
There was a problem hiding this comment.
@blueimp drwaImage may throw several exceptions, like fro too huge images.
In case that happens the Promise should reject or the callback api should have an type === 'error'.
How is the best way to move this error up?
There was a problem hiding this comment.
For the Promise style API, if the result is an error, it should always reject, so your proposed solution is correct.
js/load-image.js
Outdated
| @@ -45,6 +45,9 @@ | |||
| // Not using Promises | |||
| if (resolve) resolve(img, data) | |||
| return | |||
| } else if (img instanceof Error) { | |||
| reject(img); | |||
| return; | |||
There was a problem hiding this comment.
@blueimp reject was never called here
There was a problem hiding this comment.
I did not consider exceptions in the transformation call previously, so that's a necessary addition now that exceptions can bubble up through the resoleWrapper as well.
|
|
||
| try { | ||
| loadImage.transform(img, options, callback, file, { | ||
| originalWidth: img.naturalWidth || img.width, | ||
| originalHeight: img.naturalHeight || img.height | ||
| }) | ||
| } catch (error) { | ||
| callback(error) | ||
| } |
There was a problem hiding this comment.
@blueimp This should now hanlde any uncaught exceptions in any transformations, if I understood the system correctly 🤔
Edit: Meanwhile I made a local overwrite and it catches all uncaught exceptions.
There was a problem hiding this comment.
Yes, that's the right location for the try...catch wrapper. :)
There was a problem hiding this comment.
Great. Any tests you would like to be added?
There was a problem hiding this comment.
I think it's fine to include this with just the existing tests.
Although it would be good to test this specific scenario (huge canvas sizes) as well, the tradeoff would likely be largely increased memory requirements for running the tests.
There was a problem hiding this comment.
One other item I'm still thinking about though is that the Promise executor already has an implicit try...catch around it, so it's only necessary for the callback style.
I might add this improvement after the merge though, since it requires some restructuring.
There was a problem hiding this comment.
hmh, I used the Promise API, but .catch() hanlder wasn't called 🤔
There was a problem hiding this comment.
You're right, the Promise executor cannot catch errors that happen asynchronously.
So everything is fine as is. :)
|
Thanks for your report and for providing a solution @AndyOGo. :) |
|
One last request, please squash your commits into a single one (technically this is possible via GitHub's UI as well, but that would remove your authorship information from the commit, which I wanna avoid). |
Revert "fix: uncaught exceptions, still need to reject promise" This reverts commit 5faa5f6. fix: catch uncaught error in scale transformation fix: resolveWrapper never rejected errors Revert "fix: catch uncaught error in scale transformation" This reverts commit 8e93865. fix: catch any transformation error fix: prettier
41b4e99 to
8e25a70
Compare
|
@blueimp Thank you for your feedback. |
|
Thanks again! 👍 |
fixes #110