Open zemccartney opened 6 years ago
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?
In regards to the environment vars, we currently just have these in travis:
AWS_ACCESS_KEY_ID
AWS_REGION
AWS_SECRET_ACCESS_KEY
SENDER_EMAIL_ADDRESS
TEMPLATE_DIRECTORY
Looks like we just need to add these two for the tests to pass:
MAILGUN_API_KEY
MAILGUN_SENDING_DOMAIN
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,
sendMany
more closely resemble how it was before, hopefully a bit cleaner.FINALLY (sorry to drag on here :) ), to your comment about setting new env vars, that's correct, but you'll also need to add MAILGUN_AUTHORIZED_RECIPIENT
This is a bit kludgy, I admit, but is used in testing only. Because Mailgun doesn't have a success simulator equivalent that I know of and to send to any inbox while in sandbox mode you need to invite that inbox to be an authorized recipient from your Mailgun dashboard, I've been authorizing a personal email as a recipient, then setting the authorized recipient var to that inbox. Does that work ok for you all?
Is this ready to be merged? I'd love to use this in a current project!
@zemccartney code is looking good. π @samhstn thanks for reviewing with a fine-tooth-comb. β€οΈ @shaneparsons we need to ensure that we have the necessary environment variables defined on TravisCI to ensure that this build will always pass on CI ... we need to add an "admin task" for this. β³
@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
π 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 npm audit
(just tap-spec
needed bumping).
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 sendemail
from require
's cache and re-requiring it (which is what we do now in the main test file). Not strictly necessary at all! Happy to switch back to how things were, just something I felt was an improvement.
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 mailgun-js
uses the class
syntax which is not supported on node
v4, (see: https://stackoverflow.com/questions/41960142/block-scoped-declarations-not-yet-supported-outside-strict-mode)
What do you think we should do about this? Upgrade version or look for another solution?
@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 :trollface: 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.
Sorry if I'm overstepping here! I'll happily butt right back out if so
@zemccartney we agree. LTS is the preferred support target.
Our original reasoning for having explicit Node v4 support was AWS Lambda.
We used sendemail on v4 on Lambda.
The current version of Node.js on AWS Lambda is 8.10
https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html
I think we can safely discontinue support for node.js v.4 and even v.6 on the basis that v8.10 is available.
@samhstn @nelsonic heyo! any word on moving this forward? not being impatient, no rush from me, just I realized I'd spaced on this, figured worth doubling back to see if there were anything I could do to help wrap this up. Thanks!
@nelsonic for our tests to pass, it looks like we need to set these two environment variables in .travis.yml
:
MAILGUN_API_KEY
MAILGUN_SENDING_DOMAIN
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 MAILGUN_AUTHORIZED_RECIPIENT
(see https://github.com/dwyl/sendemail/pull/83/files#diff-0fd0e07cf6d02bf7cf00f18cebb8e6eaR97)
This is a bit kludgy, I admit, but is used in testing only. Because Mailgun doesn't have a success simulator equivalent that I know of and to send to any inbox while in sandbox mode you need to invite that inbox to be an authorized recipient from your Mailgun dashboard, I've been authorizing a personal email as a recipient, then setting the authorized recipient var to that inbox. Does that work ok for you all?
@nelsonic @samhstn heyo! sorry, been a while. just following up on this. is there anything I can do to help move this forward? as always, not to pressure at all. More to keep myself from forgetting :) Thanks!
@nelsonic @samhstn hey y'all, checking back in on this. anything I can do here to move this forward?
I hope y'all've been well!
Awesome, glad to hear, thanks for reviewing! Anything I can help with on the env var front?
@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! π