Skip to content

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.

Sign up for GitHub

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

Open
mdqst wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −2
Open

fix: logger initialization, async/await usage, and error handling #262

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35bee4d
fix: logger initialization, async/await usage, and error handling
mdqst Apr 15, 2025
f7ed38e
Update app.ts
mdqst Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions apps/node/src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ import { Logger } from "@uma/logger";

dotenv.config();

const logger = Logger;
const logger = new Logger();
Copy link
Contributor

@james-a-morris james-a-morris Apr 16, 2025

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.

Copy link
Author

@mdqst mdqst Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.


async function run() {
const { APP } = process.env;
assert(APP, 'Specify the application to start with "APP=appname pnpm start"');
switch (APP) {
case "template":
void (await Template.Main(process.env));
Copy link
Contributor

@james-a-morris james-a-morris Apr 15, 2025

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

Copy link
Author

@mdqst mdqst Apr 15, 2025

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.

Copy link
Contributor

@james-a-morris james-a-morris Apr 16, 2025

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

await Template.Main(process.env);
return "Example template app running";
case "indexer":
void (await Indexer.Main(Indexer.envToConfig(process.env), logger));
Expand All @@ -36,4 +36,6 @@ async function run() {

run()
.then((x) => x && logger.info({ at: "app", message: x }))
.catch(console.log);
.catch((error) => {
logger.error(error);
});
Comment on lines +39 to +41
Copy link
Contributor

@james-a-morris james-a-morris Apr 18, 2025

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

Suggested change
.catch((error) => {
logger.error(error);
});
.catch((error) => {
logger.error({
message: "uncaught error",
at: "node::run",
error
});
});