Send single mail #327
Send single mail #327
Conversation
|
It looks like you are heading in the right direction :)
Thanks! |
thinkingserious
added
type: community enhancement
| this.setReplyTo = function(reply_to) { | ||
| this.reply_to = reply_to; | ||
| }; | ||
| Mail.prototype.addFromEmail = function (email) { |
There was a problem hiding this comment.
Just a suggestion, I think it would be good to stick with the previous convention of setX method signature for properties that are not arrays, and addX for arrays.
There was a problem hiding this comment.
And also, Email in the method name might be redundant.
There was a problem hiding this comment.
@thinkingserious I just realized that my suggestions are contrary to what you suggested in #312. 😛
Is that method signature of addFromEmail in line with what the other libraries are using?
There was a problem hiding this comment.
Hi @heitortsergent! I thought about it but followed the suggestions of @thinkingserious. Although I agree with you that we should use setX for the from email, subject, etc. This was just a proof of concept. 😃
There was a problem hiding this comment.
+1 on the setX and addX
+1 on addFrom instead of addFromEmail
Thanks!
|
I've signed and sent the CLA @thinkingserious and hope to find some time this weekend to keep working on the refactor. |
Puigcerber
force-pushed
the
send-single-mail
branch
from
0694e8f to
03e54d3
Compare
|
Hi @thinkingserious @heitortsergent! I've finalized the refactor to pass the second test. Let me know what do you guys think. |
|
@Puigcerber nice work! 💥 I need a little bit more time to go through all the changes, but it looks good to me. One quick question, did you have any specific reason for moving the functions to use |
|
@thinkingserious thinking about email best practices and the example you posted on #312, should we add an abstraction to
Are both valid options? |
|
Hi @heitortsergent! As I wiped out the previous code and started fresh, I tend to favor prototype methods over constructor methods. If we were to send multiple emails creating instances of the Mail object, memory consumption should be lower with the prototype shared among all the instances. For the other question I tried to be as flexible as possible, so the following will work: mail.addTo('example@email.com');
mail.addTo('example@email.com', 'Example Name');
mail.addTo({ email: 'example@email.com', name: 'Example Name'}); |
|
Hey! What's the status on this @thinkingserious? |
|
Hi @Puigcerber, Thanks for checking in! This PR is currently #21 in our backlog. It can rise faster in our queue if others will +1 this PR. |
|
Hello @Puigcerber, This issue has finally made it to the top of my queue. Before I proceed, I'd like to get feedback from @adamreisnz, as he has offered to help us refactor our mail helper based on his repo: https://github.com/adamreisnz/sendgrid-mailer With Best Regards, Elmer |
|
And working on the refactoring has nearly reached the top of my queue as well! |
|
Hello Everyone, I thought you might be interested in the re-design on the SendGrid Node.js Mail Helper: #378 Your feedback would be greatly appreciated! With Best Regards, Elmer |
|
This PR is moved to here: #378 |
Hi @thinkingserious! I've found some time to start working on #312 but before keep going I want your opinion because I wiped out the mail helper to start fresh with a class that simplifies the API.
There was only two tests for the mail helper and with these changes the first one with the minimum to send a single mail pass. So if you consider this is the way to go I can work on the Mail class to pass the second test.