-
Notifications
You must be signed in to change notification settings - Fork 59
feat: download from API's signed URLs instead of proxied routes #676
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: download from API's signed URLs instead of proxied routes #676
Conversation
commit: |
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.
No issues found across 5 files
|
|
||
| const attachmentsWithContent = []; | ||
| for (const attachment of apiResponse.data.data) { | ||
| const downloadResponse = await fetch(attachment.download_url); |
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.
Unlike this.resend.get, which wraps its internal fetch calls with a try/catch and normalizes network errors into the standard response format, this direct fetch invocation can throw under certain edge cases like DNS resolution failures, offline status, or other low-level network issues.
If unhandled, these exceptions would bypass the method’s usual error normalization flow, which may conflict with its original design of consistently returning an { data, error } structure.
I'm not sure if the omission of the try/catch blocks was intentional; if not, I would consider wrapping it with them to avoid unexpected errors being thrown.
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.
Great point!
7ed63ca to
fe86a2a
Compare
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.
No issues found across 5 files
b3cfc07 to
b39a39b
Compare
| download_url, | ||
| ...otherFields | ||
| } = attachment; | ||
| const downloadResult = await this.downloadAttachment(download_url); |
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.
Not sure if performance is a concern for this, but you might want to put this under a Promise.all or Promise.allSettled so that all the attachments are downloaded in parallel.
I missed that during my first review, sorry 😥
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.
Yeah, we discussed this internally as I was reviewing with @vieiralucas. For now we're going to download sequentially. The reason being that we want to review whether the SDK should download attachments at all. We're inclined to consider the download part as something that the user controls. That way they can download in whichever way they find best.
For now we decided to do it sequentially because it's a safe default. We're going to discuss this again today in one of our syncs.
b39a39b to
6f5b97d
Compare
6f5b97d to
2934e81
Compare
Before https://github.com/resend/resend-api/pull/2199 we were proxying contents directly through our API. For various reasons related to reliability and overall separation of concerns, we started using signed URLs as explained in the aforementioned PR.
This PR makes our SDK automatically download from those URLs and encode content as base64 so it can automatically be used by the
resend.sendmethod.I'm also bumping the version here so we don't have to do a separate PR for that.
Summary by cubic
Switch SDK attachment downloads to API-provided signed URLs, then return base64 content to preserve the SDK response shape. Aligns with PRODUCT-796 by removing proxied content for better reliability.
New Features
Dependencies