-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: ensure loggingLevels
is respected
#10308
Conversation
fix: ensure `loggingLevels` is respected, import
1985601
to
9e71279
Compare
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.
Looks good. I have just a few nit-picks if you want to improve the logError
function but not necessary.
if (level) { | ||
payload.logger[level]( | ||
level === 'info' | ||
? { msg: typeof err === 'object' && 'message' in err ? err.message : '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.
I always try to avoid double ternaries as a general rule.
import type { Payload } from '../types/index.js' | ||
|
||
export const logError = ({ err, payload }: { err: unknown; payload: Payload }): void => { | ||
let level: pino.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.
The type should be pino.Level | false
, if we were in strict mode L15 would error since it doesn't match the loggingLevels
type from the config.
Issue #10272 Adds `logError` utility that can be used across the codebase for logging errors
Issue #10272
Adds
logError
utility that can be used across the codebase for logging errors