search

orliesaurus / nodemailer-mailgun-transport

nodemailer is an amazing node module to send emails within any of your nodejs apps. This is the transport plugin that goes with nodemailer to send email using Mailgun 🔫
MIT License
880 stars 97 forks source link

feature: support mailgun templates #80

Closed captaincaius closed 3 years ago

captaincaius commented 5 years ago

This transport supports templating in-house, but not pointing to one of mailgun's own templates.

A quick one-line hack to whitelist 'template' seems to work on our end, but I believe that would break the existing template functionality, so it would need either:

  1. pre-remove that template field if it's an object (kinda hacky)
  2. rename the existing template key to something else (backwards breaking)
  3. both (support "template" while deprecating it and encouraging the new key)

I'd be happy to PR but need some decisions made first.

I'm guessing you'll say option 3, so if so, what do you want the new name to be?

Thanks

orliesaurus commented 5 years ago

Need more information: what are mailgun's own templates?

captaincaius commented 5 years ago

Sure no problem.

You can save templates in mailgun's UI and then just pass the template name on "template" as a string along with the template variables in json as h:X-Mailgun-Variables. See here: https://documentation.mailgun.com/en/latest/user_manual.html#templates

The cool thing about doing it this way is that someone can iterate on your email templates without needing to touch any code.

I did a little hack to allow using it this way... https://github.com/captaincaius/nodemailer-mailgun-transport/commit/60379c900698192f7fd7c58ea7347ba2229b7c42 ...and it works, BUT it probably breaks your existing templating because there's a collision in the key name (template).

As I said I'd be happy to help, but I'd need to know which of the 3 options you decide on.

orliesaurus commented 5 years ago

The honest answer is I don't know. I feel like the 1) option is the right option but I am not sure how many people are currently using the current template feature. I wish there was a way to tag @here all people that use this package in NPM and ask their thoughts :) ahaha 🤔 🤔 🤔

orliesaurus commented 5 years ago

@captaincaius 3 👍

harrytang commented 5 years ago

@here

captaincaius commented 5 years ago

PR'ed in #82 - I went with option C as discussed, and documented it :D

captaincaius commented 3 years ago

@orliesaurus I see you closed #82 ... was this addressed some other way? Or is nodemailer-mailgun-transport not going to support mailgun templates?

orliesaurus commented 3 years ago

@captaincaius If you can fix the conflicts, I am happy to take a look since there's been a rewrite :)

captaincaius commented 3 years ago

@orliesaurus nice rewrite!

I ported the tests over and to my surprise I noticed it supports both options on the template key already (i.e. option 1) :+1:

So for the new PR I'll just add the one test, a little note in the README, and maybe a little comment.