Sendgrid-nodejs: sendgrid doesnt allow readable stream as email attachments

[kksharma1618]

Sendgrid currently doesn't allow readable stream as email attachments.

From the examples: https://github.com/sendgrid/sendgrid-nodejs/blob/77e49732bc8912e8b2b66a182d38794ce672c03d/USE_CASES.md#attachments

var attachment = new helper.Attachment();
var file = fs.readFileSync('my_file.txt');
var base64File = new Buffer(file).toString('base64');
attachment.setContent(base64File);
attachment.setType('application/text');
attachment.setFilename('my_file.txt');
attachment.setDisposition('attachment');
mail.addAttachment(attachment);

fs.readFileSync in production app is a really bad idea.
It will be helpful if we can attach readable stream to helper.Attachment. It should stream the file content directly to sendgrid server, instead of pulling it in a buffer first.

See https://github.com/sendgrid/sendgrid-nodejs/pull/219#issuecomment-307537586

[thinkingserious]

Hello @kksharma1618,

Thanks for the feedback! I've added this to our backlog for implementation.

With Best Regards,

Elmer

[TunedMidja]

I'd like to implement this one!

[mbernier]

Please do @TunedMidja!!

[RaulTsc]

Would love for this to be implemented, kind of stupid that we have to encode attachments in base64 format.

[TunedMidja]

It has been implemented in this PR:
https://github.com/sendgrid/sendgrid-nodejs/pull/514

You have to ask SendGrid why it hasn't been deployed yet (status has been
"ready for deploy" for 4 months).

/ Fredrik

On Mon, Jul 9, 2018, 11:01 Raul Tomescu [e-mail] wrote:

What is needed for this to be implemented? Willing to accept a PR for it?
Hard to believe it's not been implemented 1 and a half years after it was
reported. It also looks like a common production use case.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/sendgrid/sendgrid-nodejs/issues/394#issuecomment-403410148,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AMBukE9ys_36rTmew54NxIr5uGRszlYlks5uExvNgaJpZM4N2GGl
.

[thinkingserious]

Hi @RaulTsc,

Thanks for taking the time to comment on this issue. I've added your vote to this issue in our backlog.

One of the reasons this one is still relatively de-prioritized with respect to our entire backlog is that there is currently a workaround. That said, we do plan to merge that PR as soon as we can.

With Best Regards,

Elmer

[esco]

Hi @thinkingserious, what would that workaround be?

[thinkingserious]

Hello @esco,

Here you go. Thanks!

With Best Regards,

Eler

[bathos]

Could this be reopened? The #514 PR was about accepting a buffer. It still isn’t possible to stream attachments with this client as far as I can tell.

I should note that, in case there’s some confusion here: the test that PR included is mislabeled. The test is passing the return value of readFileSync, which is not a stream. (A test that’s actually about stream support would also not be synchronous.)

https://github.com/sendgrid/sendgrid-nodejs/blob/b3b2092b7a150ffc5d68c9bb6575810a5827f69e/packages/helpers/classes/attachment.spec.js#L28-L35

The changelog incorrectly reports readable stream support as well, so even if support for streaming isn’t added, the misinfo is worth addressing:

PR #514: Allow readable stream as email attachments. Big thanks to TunedMidja for the PR!

(Would also add that the reason someone would want to pass a stream is about throughput and memory usage, not convenience. So a solution that just accumulates the total stream contents into a buffer held in memory rather than taking advantage of it to stream all the way to the network level wouldn’t be worthwhile. My guess is that the sendgrid client was just not built with these considerations so the scope of the request might be too large. Even if the issue is not reopened, though, it would be helpful if docs did not claim the support exists.)