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(common): when passing class functions to logger methods #12561

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Oct 13, 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?

this a fix for the regression introduced by PR #11281

For example:

import { Logger } from '@nestjs/common'

class Foo {}

function foo() {
  return 'foobar'
}

const logger = new Logger()
logger.log(foo)
logger.log(Foo)
logger.log(class {})

You'll see:

/tmp/nestjs-app/node_modules/@nestjs/common/services/console-logger.service.js:150
            ? this.stringifyMessage(message(), logLevel)
                                    ^
TypeError: Class constructor Foo cannot be invoked without 'new'
    at ConsoleLogger.stringifyMessage (/tmp/nestjs-app/node_modules/@nestjs/common/services/console-logger.service.js:150:37)
    at ConsoleLogger.formatMessage (/tmp/nestjs-app/node_modules/@nestjs/common/services/console-logger.service.js:142:29)
    at /tmp/nestjs-app/node_modules/@nestjs/common/services/console-logger.service.js:131:43

What is the new behavior?

for the above code you will get this now:

image

Does this PR introduce a breaking change?

  • Yes
  • No ~ in the past (nest v9) we would see the whole class serialized. As this was broken since v10.0.0, I don't think that changing the format is a breaking change for v10 but I might be wrong. I can change the display format tho

@coveralls
Copy link

coveralls commented Oct 13, 2023

Pull Request Test Coverage Report for Build c78498b0-0256-4625-9ce7-fee1b2f092b4

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.639%

Totals Coverage Status
Change from base Build 79434c05-5367-4a7a-a04c-0f91cc807577: 0.0%
Covered Lines: 6469
Relevant Lines: 6983

💛 - Coveralls

@kamilmysliwiec kamilmysliwiec merged commit 55c6a9a into nestjs:master Oct 23, 2023
5 checks passed
@kamilmysliwiec
Copy link
Member

LGTM

@micalevisk micalevisk deleted the fix/logger-print-classes branch October 23, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants