Skip to content
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

level-change workaround doesn't work when once is used instead of on #1572

Closed
bienzaaron opened this issue Oct 17, 2022 · 4 comments · Fixed by #1576
Closed

level-change workaround doesn't work when once is used instead of on #1572

bienzaaron opened this issue Oct 17, 2022 · 4 comments · Fixed by #1576

Comments

@bienzaaron
Copy link
Contributor

There is a workaround for the level-change event described here, which allows users to ignore level-change events created by child loggers. It appears this doesn't work when using once instead of on.

Working code (Using on)

The given code sample using on works fine:

const logger = require('pino')()
logger.on('level-change', function myHandler(lvl, val, prevLvl, prevVal) {
  if (logger !== this) {
    return
  }
  console.log('%s (%d) was changed to %s (%d)', prevLvl, prevVal, lvl, val)
})
logger.child({}); // trigger an event by creating a child instance, notice no console.log
logger.level = 'trace' // trigger event using actual value change, notice console.log

This correctly logs:

info (30) was changed to trace (10)

Not working code (Using once)

The same thing does NOT work when replacing on with once:

const logger = require('pino')()
logger.once('level-change', function myHandler(lvl, val, prevLvl, prevVal) {
  if (logger !== this) {
    return
  }
  console.log('%s (%d) was changed to %s (%d)', prevLvl, prevVal, lvl, val)
  logger.once('level-change', myHandler)
})
logger.child({});
logger.level = 'trace'

This incorrectly logs:

info (30) was changed to info (30)
info (30) was changed to trace (10)
@bienzaaron
Copy link
Contributor Author

bienzaaron commented Oct 17, 2022

It looks like using on with options {once: true} works as well, but pino's types don't allow passing of the options object:

pino/pino.d.ts

Line 99 in 5aec425

on(event: "level-change", listener: pino.LevelChangeEventListener): this;

Edit: nevermind, it just doesn't respect the options param.

@mcollina
Copy link
Member

I don't think there is much we can do about this. We do not override once or on and this looks like behavior inherited from Node.js core. What we could do is to include the actual logger as an argument. Would that be enough to fix your problem?

@bienzaaron
Copy link
Contributor Author

Ah, I see. I think that should work. I can work on a PR to add it as an argument.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants