Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transport can only be configured through config #6

Open
knownasilya opened this issue Nov 9, 2016 · 8 comments
Open

Transport can only be configured through config #6

knownasilya opened this issue Nov 9, 2016 · 8 comments

Comments

@knownasilya
Copy link
Member

This line https://github.com/denali-js/denali-mailer/blob/master/lib/mailer.js#L43 shows that a transport is coming from app/mail-transports/application.js most likely, but if you don't have that it uses the config. Seems like logic is missing for using custom transports outside of the config.

@davewasmer
Copy link
Contributor

Actually it's really using the container as a kind of cache there. Basically, line 43 to 52 is there to ensure that all the mailers use the same transport. Since they all have access to the same container, it uses that to store the transport once it's instantiated. The intention wasn't to allow you to define an app/mail-transports/application.js.

That's certainly something we could consider though - is there anything in particular you had in mind that you'd need app/transports/application.js for instead of just defining in config?

@knownasilya
Copy link
Member Author

knownasilya commented Nov 9, 2016

is there anything in particular you had in mind that you'd need app/transports/application.js for instead of just defining in config?

Not really. Do you foresee a need to have multiple transports for different mailers? Also, should mailers be singletons initialized automatically at run time?

@davewasmer
Copy link
Contributor

Do you foresee a need to have multiple transports for different mailers?

Probably at some point. I could imagine having a "transactional" email service, and a separate "marketing/newsletter/blast" email service. That's probably a more advanced use case, so I held off on implementing that for now. When that is needed, we could probably do something like allow mailers to define their own transport property or something.

Also, should mailers be singletons initialized automatically at run time?

No, the Mailer class isn't a singleton. A new instance is created for each email sent out. A Mailer class basically represents the "template", meaning the actual text / html, as well as whatever data fetching logic is necessary to populate that template that isn't unique to the recipient.

That last caveat is because emails often need to be populated with two kinds of data: data that is unique to the recipient (i.e. a salutation, "Hey Ilya!"), and data that isn't unique to the recipient, but isn't part of the static template (i.e. new forum posts in the past week).

The non-unique-to-recipient kind of data lookup logic belongs in the Mailer class for that email. That way, you centralize the "find new forum posts" logic in one place rather than duplicating it everywhere you might send that email from.

The unique-to-recipient belongs wherever the Mailer class is triggered from. For example, a background job for your weekly digest email looks up the list of current users, then fires a mailer off for each one with the user data it looked up. No reason for the mailer to lookup the same data about the user again.

@knownasilya
Copy link
Member Author

I can see us duplicating this addon for sms..

@davewasmer
Copy link
Contributor

Hm, good point. I suppose SMS could be just another transport - perhaps "denali-messenger" instead of "denali-mailer" lol

@davewasmer
Copy link
Contributor

Push notifications too

@davewasmer
Copy link
Contributor

davewasmer commented Nov 9, 2016

Ha - we should include a lob.com transport for actual snail mail lol

@knownasilya
Copy link
Member Author

Lol, lob.com is cool 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants