Skip to content

Conversation

@NicolasMarino
Copy link

πŸ”— Linked issue

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR refactors the HTTP-based mail transports to centralize configuration validation and base URL normalization.

Key Improvements:

  • Centralized logic: Introduced validateConfig and normalizeBaseUrl helpers in src/utils.ts to ensure consistency and reduce code duplication across transports.
  • Improved Reliability: Refactored Resend, Mailgun, SparkPost, and Brevo transports to use these centralized helpers.
  • Robust Error Handling: Moved configuration validation inside the send method's try/catch block. This ensures that any missing critical configuration (like the API key or baseUrl) is caught and reported safely through the transport's callback, preventing unhandled exceptions.
  • URL Normalization: Standardized the formatting of base URLs to handle trailing slashes consistently across all services.
  • Test Coverage: Added integration tests for the Resend transport specifically to verify that validation errors are correctly triggered when required configuration fields are missing.
  • Security: Updated dependencies (like nodemailer, mjml) to their latest versions to ensure the package remains secure.

πŸ“ Checklist

@thetutlage
Copy link
Member

Looks good overall. But we should move the validation to the place where we define the config or create the transport instance, because validating within the send method delays the validation feedback loop

@NicolasMarino
Copy link
Author

You are right, that makes sense. I have fixed it to use constructors, added the corresponding tests, and formatted the files. Thanks for reviewing

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

Successfully merging this pull request may close these issues.

2 participants