-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix Dynamic smtp config require server restart to reflect changes #3282
base: master
Are you sure you want to change the base?
Fix Dynamic smtp config require server restart to reflect changes #3282
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
All contributors have signed the CLA ✍️ ✅ |
Quality Gate passedIssues Measures |
I have read the CLA Document and I hereby sign the CLA |
@@ -76,22 +76,29 @@ export class NodemailerEmailSender implements EmailSender { | |||
} | |||
|
|||
private getSmtpTransport(options: SMTPTransportOptions) { | |||
if (!this._smtpTransport) { | |||
if (!this._smtpTransport || | |||
JSON.stringify(this._smtpTransport?.options) !== JSON.stringify(options) |
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 believe there is a theoretical risk here with calling JSON.stringify
on the options
object could throw an error if there is a circular data structure in the object. I'm not sure but the options object can potentially include some complex structures like connection
and socket
and tls
.
So it would be safer to extract a function for comparing these stringified objects which wraps the stringification in a try-catch block.
Description
Fixes the issue
When using dynamic SMTP config per channel, if SMTP config is changed from admin portal.
Nodemailer was not using updated config.
Had to restart the server to reflect the changes.