FoalTS / foal

Full-featured Node.js framework 🚀
https://foalts.org/
MIT License
1.9k stars 142 forks source link

Console.error in handle-error is a surprise #738

Closed jondot closed 3 years ago

jondot commented 4 years ago

Hi, This option here: https://github.com/FoalTS/foal/blob/master/packages/core/src/express/handle-errors.ts#L23

Is kind of a surprise. Because we do have a logger and in general literally using console is bad (just a best practice I guess, it isn't strictly bad).

Maybe use debug instead? or the build-in logger?

LoicPoullain commented 4 years ago

we do have a logger

I'm not sure to see what you are referring to. Which logger are you talking about?

in general literally using console is bad (just a best practice I guess, it isn't strictly bad).

If needed, this behavior can be overridden by setting the configuration settings.logErrors to false and using AppController.handleError (https://foalts.gitbook.io/docs/topic-guides/cookbook/error-handling). With this method, we can print the error message and stacktrace as we wish, including by creating one's own logger service.

Maybe use debug instead?

AFAK the debug module is only used when NODE_ENV=debug, i.e. when we try to debug deeply the app (for example with running the debugger, which is not suitable in a production/staging environment).

jondot commented 4 years ago

I was referring to the winston logger that already exists in Foal, and why not use that for logging errors?

Regarding debug -- yes that's what I did eventually, replaced instances of console.log and now I'm using DEBUG=foobar:* to get a granular deep overview of everything in nodejs including Foal. It works beautifully so far.

LoicPoullain commented 4 years ago

I was referring to the winston logger that already exists in Foal, and why not use that for logging errors?

The logger service shown in https://foalts.gitbook.io/docs/topic-guides/utilities/logging-and-debugging#advanced-logging is an example of how to integrate an advanced logging service in Foal. It is not built-in the framework. The main reason for this is not to add another external dependency (winston) to the framework (see https://github.com/FoalTS/foal/blob/master/.github/CONTRIBUTING.MD#dependency-policy).

I'm also not familiar with what an advanced logger could bring to the framework. Would you have some examples where a simple console.error is not enough?

Regarding debug -- yes that's what I did eventually, replaced instances of console.log and now I'm using DEBUG=foobar:* to get a granular deep overview of everything in nodejs including Foal. It works beautifully so far.

👍

jondot commented 4 years ago

I mixed up a little, the logger is morgan: https://github.com/FoalTS/foal/blob/master/packages/core/src/express/create-app.ts#L4

So what I mean is that if this thing exists, why not use it across the board even for these console.log moments.

LoicPoullain commented 3 years ago

So what I mean is that if this thing exists, why not use it across the board even for these console.log moments.

morgan is a not logger function that can be used elsewhere like console.log or winston. It's an express middleware and so it can only be used as is. Maybe Foal will have a more consistent way to log its messages in the future but this is not a priority for now. This is why I'm closing this issue.

Thank you for sharing your experience on this 👍

LoicPoullain commented 1 year ago

Update here: starting from v4.1, Foal has an advanced system for logging: https://foalts.org/docs/common/logging/. Errors are now logged with Logger.error.