-
Notifications
You must be signed in to change notification settings - Fork 2
fix: logger initialization, async/await usage, and error handling #262
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
base: master
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.
Thanks for the PR - one question
assert(APP, 'Specify the application to start with "APP=appname pnpm start"'); | ||
switch (APP) { | ||
case "template": | ||
void (await Template.Main(process.env)); |
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 the void
is to prevent the linter from throwing a warning that we have a value that isn't being saved to a var
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.
When you write await Template.Main(process.env)
, the result of that operation isn’t stored, and the linter might complain because the return value is unused. In such cases, using void
before the expression tells the linter that you’re intentionally ignoring the result, which suppresses the warning.
So, if you choose to keep void
, it’s a valid way to silence the linter while making it clear that the return value doesn’t matter.
If the goal is to avoid linter warnings and you don’t need the result of the call, then keeping void
makes sense. On the other hand, if the linter doesn’t complain, it’s also fine to leave it out.
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.
Hey! This is correct - we're intentionally discarding the output
apps/node/src/app.ts
Outdated
dotenv.config(); | ||
|
||
const logger = Logger; | ||
const logger = new Logger(); |
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.
Hmm - I think Logger
(where we get it from) is an already instantiated extension of winston
. I think adding the new
would remove the slack/pager duty specific transports.
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.
fixed.
.catch((error) => { | ||
logger.error(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.
Thoughts on something like this? Otherwise we may have issues with the UMA/logger package
.catch((error) => { | |
logger.error(error); | |
}); | |
.catch((error) => { | |
logger.error({ | |
message: "uncaught error", | |
at: "node::run", | |
error | |
}); | |
}); |
fixed a few issues in the code:
the
logger
variable wasn't properly initialized. It was assigned theLogger
class itself, but it needs to be an instance of the class. now it's correctly initialized asconst logger = new Logger();
.removed the unnecessary
void
in front of theawait
expression. usingawait Template.Main(process.env)
is sufficient to ignore the result if needed, without wrapping it invoid
.the error handler was too basic. Instead of logging errors directly to the console, I added a more structured handler:
catch((error) => { logger.error(error); })
, so that errors are logged using the logger instance instead of just being printed out.