-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Feat/image loading errors #858
base: main
Are you sure you want to change the base?
Conversation
bitbeckers
commented
Jun 20, 2023
- Added 400 error to CORS when URL missing
- Added catch to html2canvas async call
- Error loggin and toast
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -516,7 +522,18 @@ const exportAsImage = async (id: string) => { | |||
//useCORS: true, | |||
proxy: "https://cors-proxy.hypercerts.workers.dev/", | |||
imageTimeout: 0, | |||
}).catch((e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that html2canvas will throw an error if fetching the image fails? Worth some testing either manually or via unit/integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually now that I think about it, I don't think this solves the issue, because without the catch any thrown errors should stop everything. Instead the behavior we see is that the hypercert mints anyway with the broken images.
So catching is insufficient....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep found the same during testing rn. I'm going to think about an alternative route. Because it's not like the canvas will be empty (that'd be easy to check), it's missing one specific layer
}); | ||
|
||
if (!canvas) { | ||
return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should show an error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Moving this PR to #1145 as this has become stale. |
@holkeb have you heard anything about this? |
Moving to backlog until further notice, as no requests or messages relating to this issue came to our attention since work was commenced on this. |