-
-
Notifications
You must be signed in to change notification settings - Fork 609
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
added SMTP support and removed resend dependency #313
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as these changes, we will need to update the following docs
- Contributing guide
- The docker repo: https://github.com/triggerdotdev/docker
- The .env.example
- The fly.io repo: https://github.com/triggerdotdev/fly.io
- The fly.io self hosting guide: https://trigger.dev/docs/documentation/guides/self-hosting/flyio
- The Render self hosting guide: https://trigger.dev/docs/documentation/guides/self-hosting/render
You may notice some issues with SMTP depending on provider. Might be worth enabling pooling if that happens (yes, that's my code word now). |
Lol, I'll definitely look into this. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works! I had to make a small change to coerce the env var string to a number
@Chigala to merge this that checklist above from @ericallam needs to be completed |
@matt-aitken I've already sent pull requests to the docker and fiy.io repositories. |
packages/emails/src/index.tsx
Outdated
from: string; | ||
replyTo: string; | ||
}) { | ||
this.#client = config.smtpConfig ? nodemailer.createTransport(config.smtpConfig) : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry but I don't quite understand this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericallam Should I put back the resend api key and dependency as a fallback to when the smtp config doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Eric just meant the config values should be checked instead of the entire object. Otherwise, this will never return undefined
. Checking for truthy smtpConfig.host
should probably be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, makes sense. Thanks
@Chigala when you get a chance could you check my comment above? |
is this planned? I can recreate the PR and update the code if @Chigala is not available |
also happy to help work on a revival of this 🙏 |
Description
Added SMTP support
Changes
fixes #282