Add Support for Alternate Email Transports #1580
Conversation
Signed-off-by: Erin Allison <[email protected]>
Signed-off-by: Erin Allison <[email protected]>
Signed-off-by: Erin Allison <[email protected]>
It apparently causes issues when transpiled/bundled Signed-off-by: Erin Allison <[email protected]>
Signed-off-by: Erin Allison <[email protected]>
Signed-off-by: Erin Allison <[email protected]>
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
internal-packages/emails/src/transports/aws-ses.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a comprehensive overhaul of the email transport system in the Trigger.dev application. The changes replace the Resend-specific email sending mechanism with a more flexible, modular approach using Nodemailer. The implementation now supports multiple email transport options including Resend, SMTP, and AWS Simple Email Service (SES), with enhanced configuration flexibility through environment variables and a new transport-agnostic email client architecture. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
internal-packages/emails/src/transports/aws-ses.ts (1)
13-22: Consider configuring the SES client
If you need to specify a region or credentials explicitly, consider passing them tonew awsSes.SESClient(). Otherwise, depending on environment defaults might be acceptable.apps/webapp/app/services/email.server.ts (1)
35-65: Robust transport configuration
The switch statement correctly handles all transport types, and logging clarifies the chosen transport. One suggestion:
- Confirm environment variables for AWS region or credentials if needed.
Otherwise, this is a clear and extensible solution.
internal-packages/emails/src/index.tsx (1)
47-54: Constructor consistently applies config
Good job defaulting to a null/undefined transport when no type is specified.internal-packages/emails/src/transports/null.ts (1)
9-10: Unnecessary constructor
You can remove this empty constructor without impacting functionality.- constructor(options: NullMailTransportOptions) { - } + // Remove the constructor entirely for cleanliness🧰 Tools
🪛 Biome (1.9.4)
[error] 9-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
internal-packages/emails/src/transports/resend.ts (1)
11-16: Instantiating the Resend client
Straightforward approach. Consider validating theapiKeyif it is truly required..env.example (2)
38-44: Consider adding email address format examples.The Resend configuration section is well-documented with clear instructions and links. Consider adding example formats for
FROM_EMAILandREPLY_TO_EMAILto guide users.# FROM_EMAIL= +# [email protected] # REPLY_TO_EMAIL= +# [email protected]
57-59: Consider adding AWS region configuration example.The AWS SES configuration is clear but could benefit from an example AWS region configuration.
# EMAIL_TRANSPORT=aws-ses # FROM_EMAIL= # REPLY_TO_EMAIL= +# AWS_REGION=us-east-1 # Optional: Defaults to AWS SDK region resolutiondocs/open-source-self-hosting.mdx (1)
303-305: Minor grammar improvement needed.The sentence structure could be improved for better readability.
-Credentials are to be supplied as with any other program using the AWS SDK. -In this scenario, you would likely either supply the additional environment variables `AWS_REGION`, `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` or, when running on AWS, use credentials supplied by the EC2 IMDS. +Credentials can be supplied in two ways: +1. Set the environment variables `AWS_REGION`, `AWS_ACCESS_KEY_ID`, and `AWS_SECRET_ACCESS_KEY` +2. When running on AWS, use credentials from EC2 Instance Metadata Service (IMDS)🧰 Tools
🪛 LanguageTool
[grammar] ~304-~304: Do not use an adverb of manner between a verb and its direct object.
Context: ...he AWS SDK. In this scenario, you would likely either supply the additional environmen...(MD_RB_DT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.example(1 hunks)apps/webapp/app/env.server.ts(2 hunks)apps/webapp/app/services/email.server.ts(3 hunks)docs/open-source-self-hosting.mdx(1 hunks)internal-packages/emails/package.json(2 hunks)internal-packages/emails/src/index.tsx(4 hunks)internal-packages/emails/src/transports/aws-ses.ts(1 hunks)internal-packages/emails/src/transports/index.ts(1 hunks)internal-packages/emails/src/transports/null.ts(1 hunks)internal-packages/emails/src/transports/resend.ts(1 hunks)internal-packages/emails/src/transports/smtp.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/open-source-self-hosting.mdx
[grammar] ~304-~304: Do not use an adverb of manner between a verb and its direct object.
Context: ...he AWS SDK. In this scenario, you would likely either supply the additional environmen...
(MD_RB_DT)
🪛 Biome (1.9.4)
internal-packages/emails/src/transports/null.ts
[error] 9-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
internal-packages/emails/src/transports/aws-ses.ts
[error] 34-34: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
[error] 52-52: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
internal-packages/emails/src/transports/smtp.ts
[error] 35-35: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
[error] 53-53: Catch clause variable type annotation must be 'any' or 'unknown' if specified.
(parse)
🪛 GitHub Check: typecheck / typecheck
internal-packages/emails/src/transports/aws-ses.ts
[failure] 34-34:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.
[failure] 52-52:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.
internal-packages/emails/src/transports/smtp.ts
[failure] 35-35:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.
[failure] 53-53:
Catch clause variable type annotation must be 'any' or 'unknown' if specified.
🔇 Additional comments (34)
internal-packages/emails/src/transports/aws-ses.ts (3)
1-9: AWS SES Imports & Type Definition Look Good
All imports and the AwsSesMailTransportOptions type appear valid. Nothing critical to address here.
24-33: Ensure consistent error handling
The method correctly logs and rethrows errors as EmailError. This approach is solid for debugging.
42-51: Double-check error handling consistency
Similar to the previous method, keep error handling consistent. Good job logging error details.
apps/webapp/app/services/email.server.ts (2)
3-3: Good import of MailTransportOptions
This ensures typed transport structuring.
Line range hint 17-28: Singleton initialization looks sound
Using the buildTransportOptions function in each singleton ensures the correct transport. No issues here.
internal-packages/emails/src/index.tsx (4)
16-16: Centralized transport construction
Bringing in constructMailTransport fosters a more modular, pluggable design.
41-42: Private transport field
Declaring #transport: MailTransport is a neat way to encapsulate transport details.
65-70: Delegating HTML email sending
Offloading detailed sending logic to the transport is clean and maintainable.
75-79: Consistent plain text email handling
Again, relying on sendPlainText from the transport ensures consistent behavior across all transports.
internal-packages/emails/src/transports/null.ts (3)
1-7: Null transport initialization
This straightforward type indicates no real sending, which is helpful in local or dev scenarios.
12-21: Logging HTML-based message
Sending to the console for local dev is an ideal fallback. Nicely done.
22-29: Logging plain text message
Same concept for plain text—simple and effective for dev usage.
internal-packages/emails/src/transports/resend.ts (3)
1-9: Resend transport configuration
Defining an optional API key addresses user flexibility.
18-33: HTML email sending
Properly logs and rethrows errors. This aligns well with the global error-handling strategy.
35-50: Plain text email sending
Matches the same pattern as HTML-based sending. Good job maintaining consistency.
internal-packages/emails/src/transports/index.ts (7)
1-6: Imports look good.
Importing specific transport classes and React types is organized well. No issues found.
7-13: Ensure consistent property naming.
All fields in MailMessage are consistently named and typed. Looks good for usage as a typed contract.
15-21: Plain text message type is straightforward.
The properties align with the minimal set needed to send a plain-text email.
23-26: Interface-based approach is excellent.
Defining a MailTransport interface provides a clean abstraction layer for different transports.
28-33: Custom error handling is clear and concise.
Using a custom EmailError class differentiates email-specific errors. Good practice.
35-40: Flexible transport options.
Union-based approach for MailTransportOptions fosters a scalable pattern for future transport expansions.
41-52: Defaulting to NullMailTransport is a helpful fallback.
This ensures that if type is undefined, the system degrades gracefully by not sending real emails.
internal-packages/emails/src/transports/smtp.ts (4)
1-3: Import statements are concise.
Using both render (for HTML content) and nodemailer is correctly set up for SMTP.
5-16: SMTP transport options are well-defined.
Optional fields and a clear type property enhance clarity for different SMTP config possibilities.
21-23: Transporter instantiation is correct.
nodemailer.createTransport(options.config) aligns with the recommended usage.
43-52: 🛠️ Refactor suggestion
Similar catch clause type.
Likewise, at line 53:
- catch (error: Error) {
+ catch (error: unknown) {
console.error(
...
);
throw new EmailError(error as Error);
}Likely invalid or redundant comment.
apps/webapp/app/env.server.ts (2)
47-56: Expanded email transport fields.
Adding EMAIL_TRANSPORT and SMTP-related fields fosters a flexible, multi-transport environment. Ensure documentation is updated to guide users on which fields must be set to avoid misconfiguration.
205-214: Alert transport configuration mirrors primary email transport.
Having a matching set of alert-specific fields keeps configuration consistent. No issues found.
internal-packages/emails/package.json (1)
12-15: New dependencies support multi-transport architecture.
@aws-sdk/client-sesandnodemailerare necessary for SES and SMTP implementations.- Type definitions for
nodemailerensure better consistency and maintainability.
Also applies to: 24-24, 31-31
.env.example (2)
34-37: LGTM! Clear introduction to email transport configuration.
The introduction clearly explains the purpose and default behavior of email transport configuration.
45-56: LGTM! Well-documented SMTP configuration.
The SMTP configuration section includes:
- Clear explanation of STARTTLS usage
- Common port number pre-filled
- All necessary configuration options
docs/open-source-self-hosting.mdx (3)
272-275: LGTM! Clear introduction to email configuration.
The introduction effectively explains the default behavior and configuration requirements.
286-289: LGTM! Excellent security explanation for SMTP.
Clear explanation of STARTTLS vs. implicit TLS, helping users make informed security decisions.
306-310: LGTM! Clear AWS SES configuration example.
The configuration example is concise and matches the .env.example format.
Gotta keep the linter happy :) Signed-off-by: Erin Allison <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal-packages/emails/src/transports/smtp.ts (1)
5-16: Enhance type safety and security for SMTP configuration.Consider the following improvements for the configuration type:
- Add runtime validation for the port number (valid range: 1-65535)
- Document the security implications of the
secureoption- Consider making
authrequired whensecureis trueexport type SmtpMailTransportOptions = { type: "smtp"; config: { host?: string; - port?: number; + port?: number & { _brand: 'ValidPort' }; // Ensure valid port range at runtime secure?: boolean; + /** When true, TLS is used. Strongly recommended for production. */ auth?: { user?: string; pass?: string; }; }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal-packages/emails/src/transports/aws-ses.ts(1 hunks)internal-packages/emails/src/transports/smtp.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal-packages/emails/src/transports/aws-ses.ts
🔇 Additional comments (1)
internal-packages/emails/src/transports/smtp.ts (1)
18-66: Verify integration with email transport system.
The implementation looks solid but let's verify its integration with the broader system.
✅ Verification successful
SMTP Transport implementation is properly integrated
The verification shows that the SMTP transport is well-integrated into the email system:
- It correctly implements the
MailTransportinterface with bothsendandsendPlainTextmethods - The transport is properly exported and registered in the factory pattern via
constructMailTransport - Configuration is properly typed with
SmtpMailTransportOptionscontaining all necessary SMTP settings (host, port, secure, auth) - The transport type is correctly configured in the environment setup (
EMAIL_TRANSPORTandALERT_EMAIL_TRANSPORTenums)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper integration with the email client system
# Verify the transport is properly exported and used
rg -l "SmtpMailTransport" --type ts
# Check for configuration handling in the main application
rg "EMAIL_TRANSPORT.*smtp" --type ts
# Look for test coverage
rg -l "describe.*SmtpMailTransport" --type ts
Length of output: 423
Script:
#!/bin/bash
# Let's check the transport configuration and integration
# Check how transports are registered and used
cat internal-packages/emails/src/transports/index.ts
# Check the environment configuration structure
ast-grep --pattern 'interface $_Config {
$$$
}'
# Look for SMTP configuration usage
rg "smtp.*config" --type ts -A 3
Length of output: 1555
Script:
#!/bin/bash
# Let's check the SMTP configuration structure and usage
# Check SMTP configuration type
ast-grep --pattern 'type SmtpMailTransportOptions = {
$$$
}'
# Look for SMTP configuration setup
rg "type.*=.*smtp" --type ts -A 3
# Check for SMTP configuration validation
rg "smtp.*validate" --type ts -A 3
Length of output: 943
| async send({ to, from, replyTo, subject, react }: MailMessage): Promise<void> { | ||
| try { | ||
| await this.#client.sendMail({ | ||
| from: from, | ||
| to, | ||
| replyTo: replyTo, | ||
| subject, | ||
| html: render(react), | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| console.error( | ||
| `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}` | ||
| ); | ||
| throw new EmailError(error); | ||
| } else { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling and add email validation.
The error handling could be more specific and include input validation.
- Add email validation
- Extract common error handling logic
- Add specific error types for different failure scenarios
+private handleError(error: unknown, to: string, subject: string): never {
+ if (error instanceof Error) {
+ console.error(
+ `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}`
+ );
+ throw new EmailError(error);
+ }
+ throw error;
+}
async send({ to, from, replyTo, subject, react }: MailMessage): Promise<void> {
+ if (!this.isValidEmail(to) || !this.isValidEmail(from)) {
+ throw new Error('Invalid email address');
+ }
try {
await this.#client.sendMail({
from: from,
to,
replyTo: replyTo,
subject,
html: render(react),
});
} catch (error) {
- if (error instanceof Error) {
- console.error(
- `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}`
- );
- throw new EmailError(error);
- } else {
- throw error;
- }
+ this.handleError(error, to, subject);
}
}Committable suggestion skipped: line range outside the PR's diff.
| async sendPlainText({ to, from, replyTo, subject, text }: PlainTextMailMessage): Promise<void> { | ||
| try { | ||
| await this.#client.sendMail({ | ||
| from: from, | ||
| to, | ||
| replyTo: replyTo, | ||
| subject, | ||
| text: text, | ||
| }); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| console.error( | ||
| `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}` | ||
| ); | ||
| throw new EmailError(error); | ||
| } else { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in error handling.
The error handling logic is duplicated between send and sendPlainText methods.
Use the same error handling approach as suggested for the send method:
async sendPlainText({ to, from, replyTo, subject, text }: PlainTextMailMessage): Promise<void> {
+ if (!this.isValidEmail(to) || !this.isValidEmail(from)) {
+ throw new Error('Invalid email address');
+ }
try {
await this.#client.sendMail({
from: from,
to,
replyTo: replyTo,
subject,
text: text,
});
} catch (error) {
- if (error instanceof Error) {
- console.error(
- `Failed to send email to ${to}, ${subject}. Error ${error.name}: ${error.message}`
- );
- throw new EmailError(error);
- } else {
- throw error;
- }
+ this.handleError(error, to, subject);
}
}Committable suggestion skipped: line range outside the PR's diff.
| constructor(options: SmtpMailTransportOptions) { | ||
| this.#client = nodemailer.createTransport(options.config); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transport validation and cleanup.
The constructor should validate the transport creation and implement proper cleanup.
constructor(options: SmtpMailTransportOptions) {
- this.#client = nodemailer.createTransport(options.config);
+ try {
+ this.#client = nodemailer.createTransport(options.config);
+ } catch (error) {
+ throw new Error(`Failed to initialize SMTP transport: ${error instanceof Error ? error.message : 'Unknown error'}`);
+ }
}
+
+async verifyConnection(): Promise<void> {
+ await this.#client.verify();
+}
+
+async close(): Promise<void> {
+ await this.#client.close();
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(options: SmtpMailTransportOptions) { | |
| this.#client = nodemailer.createTransport(options.config); | |
| } | |
| constructor(options: SmtpMailTransportOptions) { | |
| try { | |
| this.#client = nodemailer.createTransport(options.config); | |
| } catch (error) { | |
| throw new Error(`Failed to initialize SMTP transport: ${error instanceof Error ? error.message : 'Unknown error'}`); | |
| } | |
| } | |
| async verifyConnection(): Promise<void> { | |
| await this.#client.verify(); | |
| } | |
| async close(): Promise<void> { | |
| await this.#client.close(); | |
| } |
|
I'll be honest... I have no idea what's causing the typecheck to fail. I didn't know to run the typecheck command to validate, but now that I have run it, I get the same error locally. My IDE doesn't say anything's wrong and the code works. I'm guessing it's something small that I missed. Can someone have a look and point me in the right direction? |
Closes #282
Supercedes #313
✅ Checklist
Testing
Configured a local install for AWS SES and then SMTP, verifying each configuration properly sends a magic link email.
*I was unable to test Resend, as I do not have an account with them. The logic for Resend specifically, though, did not change, and it follows the same interface as the others, so there shouldn't be any issues.
Changelog
internal-packages/emailsto allow addition of alternative email transports (as opposed to just Resend API)Screenshots
I have screen recordings of the testing saved, though I won't be sharing them publicly, as credentials (though they should be invalid by now) are visible. I am happy to provide them privately to the team for further validation, if necessary.
💯
--
Breaking Change
This does introduce one small breaking change: currently to use Resend, all that needs to be specified is
RESEND_API_KEY. As a result of needing to support varying config options,EMAIL_TRANSPORT=resendwould need to be added to maintain the current behavior.If this is not done, the application will default to the behavior of printing emails to the console, as if no email transport is configured.
This could be worked around by coding a special case for Resend due to existing deployments, but I didn't think it necessary unless was that deemed to be enough of a concern.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes