Conversation
|
Ay! My bad on the Travis checks. I probably should've thought about needing to update the .env file there :) Do I need to handle updating that or do you prefer to? Whichever's good! let me know if I can help with that |
|
@nelsonic heyo! do you need anything more from me here? or all set to review? No rush at all! I know you're busy and this is a side-thing, checking in mostly so I don't forget about this. If the answer's just that I should be patient, I can do that, too :) Hope you had a great weekend! |
|
Heyo @nelsonic ! anything I can do to push this forward? again, i know it's small potatoes, no big rush, mainly curious :) Thanks again for the help here. Hope you've been having a good weekend 🙏 |
|
@samhstn @nelsonic hey! sorry, I'm super late here, but I finally cleaned up that merge conflict, all tests are back to running, and README is up-to-date. Travis checks aren't passing, I assume, because some new env vars need to be set for Mailgun. Lemme know if I can help with updating that at all (i.e. if my code / instructions ended up totally unclear). How is this looking to you? |
samhstn
left a comment
samhstn
left a comment
There was a problem hiding this comment.
@zemccartney Love the updates to the documentation, very clean with loads of little improvements 👍
Let me know your thoughts on the couple of comments I've left for you - I'm happy to approve once answered and travis is fixed
Thanks for your work so far on this ❤️
| + [ ] borrow the code for `hello.txt` and `hello.html` from the **/examples/templates** directory of this project! | ||
| + [ ] create a file called `welcome.js` and paste some sample | ||
| code in it (see: [/examples/templates/**send-welcome-email.js**]() ) | ||
| code in it (see: [/examples/templates/**send-welcome-email.js**](/examples/templates/send-welcome-email.js) ) |
| If you wish to send to multiple recipients of include CC or BCC recipients, | ||
| use the sendMany method. This allows you to provide an options object | ||
| with an array of `toAddresses`, `ccAddresses`, and `bccAddresses` and charset options. | ||
| If you wish to send to multiple recipients or include CC or BCC recipients, |
| ### Which Email Service Provider? | ||
|
|
||
| We are *currently* using ***AWS SES*** for ***dwyl***. | ||
| We also support **Mailgun** |
lib/index.js
Outdated
| var COMPILED_TEMPLATES = {}; // -> cache compiled templates | ||
|
|
||
| AWS.config.region = process.env.AWS_REGION; | ||
| var send = require('./service').send; // -> email service, for sending |
There was a problem hiding this comment.
moving aws specific requires and configuration into lib/service.js makes sense 👍
lib/index.js
Outdated
|
|
||
| ses.sendEmail(params, callback); | ||
| // Deep clones options, ensures no mutation of input (deep necessary because source may contain arrays) | ||
| var clean_options = JSON.parse(JSON.stringify(options)); |
There was a problem hiding this comment.
@zemccartney could you just explain why a deep clone was needed here? As far as I'm aware we weren't mutating the options passed in before. At which point did this become necessary? - I just think it would be great if it was possible to avoid this.
(I quite liked how easy these lines were to read before: https://github.com/dwyl/sendemail/blob/master/lib/index.js#L77-L85)
There was a problem hiding this comment.
@samhstn hey, thanks calling this out. definitely a bit of a kludge. my bad on that.
I used a deep clone here because I needed to end up with an object containing all the values of the options object that I could then pass as the first parameter to the send call. Since we're running those conditional checks and filters on the properties of that object and need to store those results, I think I thought that meant we'd be mutating the input options (sorry, it's been long enough since I wrote this that I've since forgotten / am a bit lost myself on what I was thinking :) ).
I tried to clean this up a good bit in the work I just pushed up. Still creating a new object, but not cloning our options object. Hopefully readable, little less misdirection here. How does that look?
lib/service.js
Outdated
| * @returns {Object} interface for parameterizing and sending email send requests | ||
| * with the determined service | ||
| */ | ||
| function determine_service (specific_service) { |
There was a problem hiding this comment.
I have a feeling there is maybe a nicer way to do this, having this as part of the library code and referencing another CURRENT_SERVICE env var feels a little off, let me know your thoughts @nelsonic @Danwhy @zemccartney?
There was a problem hiding this comment.
@samhstn totally agree, I was never all that happy with how I did things here.
I came up with a new approach to this in the stuff I just pushed up.
I still have the library using another env var, now renamed to SENDEMAIL_SERVICE to hopefully make the var's name more descriptive/understandable, but only if the user happens to have multiple services configured at once. In that case, we throw an error saying the user's env config is ambiguous, directing them to use that variable to tell the library which sending service to use.
In short, I tried to clarify and minimize the purpose of that env var by moving it out of the main path of the determine_service path.
I think I was solving for the, to be honest, nonsensical defaulting behavior I had in place to handle this case before (essentially, unless the user had set CURRENT_SERVICE, the library would use the last available service, in alphabetical order....no good 😆)
Does that get to root of this issue? Or at least improve things a bit?
My bad if not! I just jumped to this approach to try working through another thought.
I kept determine_service for now because I figure we need to handle the case — I hope a seriously unlikely one, but I guess still possible — that a user could configure multiple services at once, like our tests. If that's not important, I bet we can simplify this part of the library a good deal (though not sure of the clearest route for that aside from continuing to use an env var...would have to think about that)
There was a problem hiding this comment.
@samhstn yo! any thoughts here?
sorry, no rush at all. I don't mean to bug, more chiming back so wrapping this up doesn't completely fall off my radar (again :) ). Let me know if there's anything I can do to help move this forward. Thanks for all the help so far
There was a problem hiding this comment.
I've had a read through the code, thanks for the improvements here with comments and the explanations - very helpful.
I just want to raise #83 (review) as a concern before any further reviewing or work is done in this pr.
test/service.test.js
Outdated
| // TODO Delete this test / Or rather, switch to using CURRENT_SERVICE | ||
| // TODO Add a test for using CURRENT_SERVICE | ||
| // TODO Ok assumption????? | ||
| // Assumes .env used for testing has all available services configured simultaneously |
There was a problem hiding this comment.
@zemccartney what is your plan with these TODOs, are you planning to address these in this pr or should we open an issue to work on these in a later pr?
There was a problem hiding this comment.
heh...sorry for that sloppiness. those are all resolved, I did a pass over the service testing file. I added a handful of new stuff, but all things that make the tests more reliable and readable.
|
In regards to the environment vars, we currently just have these in travis: Looks like we just need to add these two for the tests to pass: |
|
Hey @samhstn ! Very cool, thanks for all those detailed comments. I appreciate you taking the time to review this work and write those up 🙏 I'm going to go back and leave responses, but I just pushed up another pass that I think addresses all the issues you called out. Basically,
FINALLY (sorry to drag on here :) ), to your comment about setting new env vars, that's correct, but you'll also need to add |
|
Is this ready to be merged? I'd love to use this in a current project! |
|
@zemccartney code is looking good. 🎉 |
|
@nelsonic awesome, glad to hear! thanks so much for the feedback 🙏 let me know if there's anything at all I can do to help with the CI task |
There was a problem hiding this comment.
@zemccartney thanks for the code changes here, finding it much nicer to read through.
These issues are a little concerning though: mailgun/mailgun.js#43 and mailgun/mailgun.js#39. It seems that the mailgun.js npm module hasn't been updated in 2 years and has security vulnerabilities.
Although we are using a small subset of what the mailgun api has to offer, we should at least investigate what the current security vulnerabilities are before merging this or perhaps consider a different library, one which is maintained.
|
👍 awesome, thanks for pointing that out @samhstn sorry for my oversight there. IIRC, way back when I first started this, I debated between using that library and this community-contributed one: https://github.com/bojand/mailgun-js I think I switched to Mailgun's official one, assuming it'd be more reliable, but looks like even Mailgun's official node tutorial directs people to use the community library :) Doi. On reviewing that library, it looks like it's still maintained (at least relatively recent issues and commits). So I'm thinking I'll switch out Mailgun's client for the community one, make whatever updates are necessary to get that working. As part of that work, I'll audit that project's dependencies, see if there are any known security issues there, contacting the maintainer about my findings as needed. How does that sound to you? |
|
Sounds good @zemccartney, yeah the community contributed npm module looks better to use 👍thanks for looking into this ❤️ |
|
@samhstn heyo! sorry for the long delay on this, hit a busy stretch. The commit I just pushed up replaces the unmaintained mailgun client with the maintained one and updates dependencies based on the results of Additionally, I noticed how I'd written things previously was a bit inefficient — I had been determining a service and instantiating a sending client on every send — so I refactored the main export of the service module (the service-specific sending function) to enclose just a call to the determined service's sending method. The upside is that this is a bit more efficient, I imagine. Downside is that you can no longer switch your sending service mid-process without clearing Any questions or anything I can do to help here, lemme know! Thanks again for all the help so far 🙏 |
|
Wicked, thanks for the feedback @samhstn 🙏 I'll address those notes soon |
|
@samhstn heyo! thanks again for all the feedback. I think I covered everything in the changes I just pushed up, but as always, let me know whatever else more is needed 🙏 |
|
@zemccartney looks great ❤️ thanks for your work @nelsonic the build is failing because What do you think we should do about this? Upgrade version or look for another solution? |
| ### Checklist (*everything you need to get started in 5 minutes*) | ||
| + [ ] install the `sendemail` module from NPM | ||
| + [ ] Ensure that you have an an account on your email service of choice | ||
| + [ ] Ensure that you have an account for your email service of choice |
|
@samhstn @nelsonic my two cents here, whatever it's worth :) this could be a good opportunity to align sendemail's node support more closely with node's current maintenance schedule i.e. since node 4 is no longer maintained, you could bump the lowest version of node supported here to 6 or 8 (both in maintenance LTS for parts of this year ; and it looks like node 6 is the earliest version to allow ES6 class syntax without a runtime flag), and release that update as sendemail v4. Users on older versions of node could still use v3 or earlier. Of course, I'm partial to this route in part because it means I don't have to do anything Sorry if I'm overstepping here! I'll happily butt right back out if so |
but I do think trying to keep step with node's maintenance schedule and the patches provided therein is a worthwhile, if exhausting, effort in the interest of both access to new features and security patches.
|
@zemccartney we agree. LTS is the preferred support target. I think we can safely discontinue support for node.js v.4 and even v.6 on the basis that v8.10 is available. |
|
@nelsonic for our tests to pass, it looks like we need to set these two environment variables in Do you want me to go ahead and try and add them (I think I have permissions issues)? |
|
@samhstn yo! thanks for jumping right on this. re: the new env vars, you'll also need to add |
|
Awesome, glad to hear, thanks for reviewing! |
| language: node_js | ||
| node_js: | ||
| - 4.3.2 | ||
| - 8.1 |
nelsonic
left a comment
nelsonic
left a comment
There was a problem hiding this comment.
Thanks @zemccartney. 👌
Apologies that this one fell off the radar. 🤷♂️
5 participants
@nelsonic heyo! got sending with Mailgun working, tests passing, all linted
I still need to cleanup the README, specifically:
but I figure I'll hold off on that final documentation cleanup until after handling whatever revisions you want here, once the code's in a place you're happy with
thanks again for letting me take this on! 🙏