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

feat(common): built-in, console logger refactor #6221

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

kamilmysliwiec
Copy link
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: N/A

#6142
#6172
#5353

and a few others.

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Other information

@coveralls
Copy link

Pull Request Test Coverage Report for Build 167df087-dfd3-42eb-87d6-1ca378e36927

  • 15 of 15 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 94.826%

Totals Coverage Status
Change from base Build 9506c706-6d5d-42ff-9f99-26bef0243d17: 0.09%
Covered Lines: 5095
Relevant Lines: 5373

💛 - Coveralls

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

Wow! This is going to be incredibly helpful to so many! Quick question about what would happen, I think I know, but want to make sure,

Say that I want to use my custom logger, so I set bufferLogs to true and will call app.useLogger(customLoggerInstance) later on. If there is an error, such as Nest not being able to resolve a dependency, because app.useLogger hasn't been called yet, it will use Nest's default ConsoleLogger to print the error, correct?

@kamilmysliwiec
Copy link
Member Author

Say that I want to use my custom logger, so I set bufferLogs to true and will call app.useLogger(customLoggerInstance) later on. If there is an error, such as Nest not being able to resolve a dependency, because app.useLogger hasn't been called yet, it will use Nest's default ConsoleLogger to print the error, correct?

Correct :)

@kamilmysliwiec kamilmysliwiec merged commit 46b1409 into 8.0.0 Jan 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/logger-refactor branch January 27, 2021 11:18
@WonderPanda
Copy link
Contributor

Late to the party but thanks for these improvements @kamilmysliwiec! This looks badass

@jjchiw
Copy link

jjchiw commented Jul 20, 2021

Hi

I don't know if it's related to this PR, but when we upgraded to nestjs 8 all the tests that usesTest.createTestingModule({ failed with this error Error: Using the "extends Logger" instruction is not allowed in Nest v8. Please, use "extends ConsoleLogger" instead.

We fixed the tests adding setLogger(new ConsoleLogger()

Test.createTestingModule({
    ...
  }).setLogger(new ConsoleLogger())
    .compile();

thanks :)

@jmcdo29
Copy link
Member

jmcdo29 commented Jul 20, 2021

@jjchiw make sure you're using @nestjs/testing@^8. V7 would still be using the old Logger class and will result in that error

@nestjs nestjs locked and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants