-
Notifications
You must be signed in to change notification settings - Fork 0
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
PDT24-45 | Fix Docker Hot-Reloading #11
base: main
Are you sure you want to change the base?
Conversation
* feat: add logging service with winston * chore(logging): use and fix logging information * PDT24-34 | Configure nodemailer service (#12) * feat: configure nodemailer servie for sending email * feat: create a basic email template for sending signup otp code * chore: rename param names and add function documentation * fix: use secure mailing port * refactor: remove sendEmail call from main and improve sendEmail method * refactor: remove ternary operator
Quality Gate passedIssues Measures |
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.
Good job - there are a few potential areas of improvement but apart from that, awesome.
new transports.File({ filename: 'logs/errors.log', level: 'error' }), | ||
new transports.File({ filename: 'logs/all.log' }), |
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.
Use { flags: 'a' }
to append to log file instead of overwriting it on each rebuild.
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.
It turns out that the existing code (without flags: 'a') works just fine. It doesn't overwrite the log files on each rebuild. so I am leaving as it is
), | ||
transports: isProduction | ||
? [ | ||
new transports.File({ filename: 'logs/errors.log', level: 'error' }), |
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.
If possible, store logs for different dates in different log files.
E.g. errors_2024-12-32.log
To stay clear of potential timezone issues, use server time or UTC.
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.
For this we'd have to install an additional package called winston-daily-rotate-file. Do you recommend to do so?
from: sender ?? { | ||
name: process.env.EMAIL_SENDER_NAME as string, | ||
address: process.env.EMAIL_USER as string, |
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.
You can define this as a constant somewhere with Address
type so it can be reused across your MailerModule
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.
instead, i removed sender as it will be constant across the application.
Tasks
Changes