apply serializers to args once before asObject or transmit #1971
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emmyakin
commented
May 12, 2024
| const argsCloned = args.slice() | ||
| let msg = argsCloned[0] | ||
| const logObject = {} | ||
| if (ts) { | ||
| logObject.time = ts | ||
| } | ||
| logObject.level = levelFormatter(level, logger.levels.values[level]) | ||
|
|
||
| if (levelFormatter) { |
There was a problem hiding this comment.
restore previous implementation to fix this bug
| @@ -361,9 +366,9 @@ function applySerializers (args, serialize, serializers, stdErrSerialize) { | |||
| for (const i in args) { | |||
| if (stdErrSerialize && args[i] instanceof Error) { | |||
| args[i] = pino.stdSerializers.err(args[i]) | |||
| } else if (typeof args[i] === 'object' && !Array.isArray(args[i])) { | |||
| } else if (typeof args[i] === 'object' && !Array.isArray(args[i]) && serialize) { | |||
There was a problem hiding this comment.
no need to start a for loop, if serialize is not true
| @@ -144,7 +144,7 @@ Unlike server pino the serializers apply to every object passed to the logger me | |||
| if the `asObject` option is `true`, this results in the serializers applying to the | |||
| first object (as in server pino). | |||
|
|
|||
| For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#parameters. | |||
| For more info on serializers see https://github.com/pinojs/pino/blob/master/docs/api.md#mergingobject. | |||
There was a problem hiding this comment.
fix docs id references
| @@ -284,7 +284,6 @@ test('children inherit parent serializers', ({ end, is }) => { | |||
|
|
|||
| test('children serializers get called', ({ end, is }) => { | |||
| const parent = pino({ | |||
| test: 'this', | |||
There was a problem hiding this comment.
this is not part of the pino config, or log parameters, seems like a mistake
| @@ -166,7 +166,7 @@ test('opts.browser.asObject logs pino-like object to console', ({ end, ok, is }) | |||
| end() | |||
| }) | |||
|
|
|||
| test('opts.browser.formatters logs pino-like object to console', ({ end, ok, is }) => { | |||
| test('opts.browser.formatters (level) logs pino-like object to console', ({ end, ok, is }) => { | |||
There was a problem hiding this comment.
test names were the same, adding context for the different formatters config
|
|
||
| end() | ||
| }) | ||
|
|
||
| test('opts.browser.write func string joining when asObject is true', ({ end, ok, is }) => { |
There was a problem hiding this comment.
this test was repeated, see line 262 below, I deleted this, git somehow is reconciling my changes adding new test using the these lines
emmyakin
commented
May 12, 2024
| @@ -302,7 +302,7 @@ function createWrap (self, opts, rootLogger, level) { | |||
| const proto = (Object.getPrototypeOf && Object.getPrototypeOf(this) === _console) ? _console : this | |||
| for (var i = 0; i < args.length; i++) args[i] = arguments[i] | |||
|
|
|||
| if (opts.serialize && !opts.asObject) { | |||
| if (opts.serialize && !opts.transmit) { | |||
There was a problem hiding this comment.
this is the required change to fix the bug
This was referenced May 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes serializers are applied twice in pino/browser #1970
applySerializersfunctioin mutatesargsto logger.*, this cause args to be updated twice inasObjectandtransmitasObjectAlso includes fix for
browser.formatters.leveldoes not inline attributes into message anymore #1966formatters.leveldoes not spread the returned object, but updateslogData.level = { level: 30, label: 'info' }