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

fix: Fix crash when setting logger to false #2225

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

ostkrok
Copy link
Contributor

@ostkrok ostkrok commented Feb 14, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When setting up the terminus module with logging disabled according to the documentation:

TerminusModule.forRoot({ logger: false })

the application will crash, with the following error:

Error starting application TypeError: this.logger.setContext is not a function
    at new HealthCheckService (/app/node_modules/@nestjs/terminus/dist/health-check/health-check.service.js:39:21)

This is caused by incorrect typing of the logger injected into HealthCheckService. It's typed to be a ConsoleLogger, which is why it looks safe to call setContext.

This is not true when passing {logger: false}, and probably not when using a customer logger either.

What is the new behavior?

Setting up the terminus module with logging disabled no longer crashes the app :)

If a ConsoleLogger is provided, we still call setContext in order to preserve the old behavior. I guess we could consider calling setContext as long as it exists on the logger object instead?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

The logger injected into the health check service was incorrectly typed,
and thus assumed to have a method that the fake logger doesn't have.
@BrunnerLivio
Copy link
Member

BrunnerLivio commented Feb 15, 2023

Oups that fell under my radar, thanks for noticing!
Released with v9.2.1 🎉

@BrunnerLivio BrunnerLivio merged commit 336bfe7 into nestjs:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants