Skip to content

Uncaught exceptions aren't catched or rejected #111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blueimp merged 1 commit into from
May 7, 2020
Merged

Uncaught exceptions aren't catched or rejected #111

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions js/load-image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
// Not using Promises
if (resolve) resolve(img, data)
return
} else if (img instanceof Error) {
reject(img)
return
}
data = data || {} // eslint-disable-line no-param-reassign
data.image = img
Expand Down Expand Up @@ -159,10 +162,15 @@

loadImage.onload = function (img, event, file, url, callback, options) {
revokeHelper(url, options)
loadImage.transform(img, options, callback, file, {
originalWidth: img.naturalWidth || img.width,
originalHeight: img.naturalHeight || img.height
})

try {
loadImage.transform(img, options, callback, file, {
originalWidth: img.naturalWidth || img.width,
originalHeight: img.naturalHeight || img.height
})
} catch (error) {
callback(error)
}
Comment on lines +165 to +173

@AndyOGo AndyOGo May 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@blueimp blueimp May 7, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the right location for the try...catch wrapper. :)

@AndyOGo AndyOGo May 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Any tests you would like to be added?

@blueimp blueimp May 7, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@blueimp blueimp May 7, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@AndyOGo AndyOGo May 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmh, I used the Promise API, but .catch() hanlder wasn't called 🤔

@blueimp blueimp May 7, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the Promise executor cannot catch errors that happen asynchronously.
So everything is fine as is. :)

}

loadImage.createObjectURL = function (file) {
Expand Down