Skip to content
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

Attachment helper for base64 encoding #504

Closed
thinkingserious opened this issue Oct 17, 2017 · 9 comments
Closed

Attachment helper for base64 encoding #504

thinkingserious opened this issue Oct 17, 2017 · 9 comments
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: twilio enhancement feature request on Twilio's roadmap

Comments

@thinkingserious
Copy link
Contributor

We have an example in USE_CASES.md on how to handle attachments.

We need to update the Attachment object such that it can also take in a raw file, rather than a base64 encoded file, and perform the base64 encoding behind the scenes.

Note that the total size of your email, including attachments, must be less than 30MB. The total size is calculated from the entire JSON payload. Note that you will need to handle the case where UTF-8 characters are included.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest labels Oct 17, 2017
@spelcaster
Copy link
Contributor

Hi @thinkingserious, I'm working in this one right now.

@spelcaster
Copy link
Contributor

See this gist, based on it, which approach do you think suits better here sample1 (read the entire file) or sample2 (read chunks)?

I'm doing this to be synchronous, is it ok?

@thinkingserious
Copy link
Contributor Author

Hi @spelcaster,

Thanks for the PR, I've added to my backlog for review. My initial thoughts is that it may be useful to provide configurable options to the user with a good default.

That said, @adamreisnz may weigh in before I get a chance to.

With Best Regards,

Elmer

@adamreisnz
Copy link
Contributor

I think that the simpler approach of reading the whole file makes sense here, because it's a helper, and for 98% of the use cases where someone just wants to include a file, it will work. With the email size limit being 30mb anyway, it's unlikely one will try to read files larger than that which would not fit in memory.

For cases where it causes problems, developers could roll their own solution and provide a buffer instead of a file.

With the configuration option and two ways of doing it I think you may run the risk of over complicating things for no real good reason, and it's possible that the chunked way of reading may still not suite everyone's needs.

That's why I think it's best to provide a simple solution that just works in most cases, while giving developers the option to build something themselves. Accepting buffers already does the latter.

Also make sure to use ES6 code style (e.g. using const, let where appropriate instead of var).

@spelcaster
Copy link
Contributor

I'll stick with the sample1.

How the Attachment should know that it needs to load the file from file system? A new prop in object passed to fromData or a new branch in the Attachment constructor?

@spelcaster
Copy link
Contributor

Follow #505

@mbernier mbernier removed difficulty: easy fix is easy in difficulty difficulty: hard fix is hard in difficulty difficulty: medium fix is medium in difficulty labels Oct 27, 2017
@thinkingserious thinkingserious added status: work in progress Twilio or the community is in the process of implementing difficulty: medium fix is medium in difficulty type: twilio enhancement feature request on Twilio's roadmap labels Mar 5, 2018
@LukeXF
Copy link

LukeXF commented Apr 3, 2018

Why do you guys require people to base64 encode files?

@thinkingserious
Copy link
Contributor Author

Hello @LukeXF,

This is a requirement of the underlying API. The purpose is this issue is to hide that implementation detail from users of this SDK.

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor Author

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. Please feel free to reopen or create a new issue if you still require assistance. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: work in progress Twilio or the community is in the process of implementing type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

5 participants