- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 69
Description
Describe the bug
The LoggerWithoutCallSite.settings
getter method merges this._parentOrDefaultSettings
and this._mySettings
in a manner which results . As of this writing, the code uses spread operators to merge the two property sets; however, this method of merging simply overwrites and does not prevent a "falsy" value from overwriting a "truthy" value. Accordingly, settings that are left undefined in a child will still overwrite the corresponding setting from the parent. The merging process must consider whether a child setting is defined prior to overwriting the defaults, or the defaults will never be used.
To Reproduce
Steps to reproduce the behavior:
import { Logger } from 'tslog';
const parent = new Logger({ name: 'parent' }); // parent.settings.name = 'parent'
const child = parent.getChildLogger({ requestId: 'foo' }); // parent.settings.name = undefined
child.info('test');
// when eventually passed through prettyPrintLog,
// parent.settings.name is evaluated to undefined and not printed
Expected behavior
Child logger settings should be merged with parent, as the documentation suggests, such that only defined settings overwrite parent/defaults.
Additional context
tslog/src/LoggerWithoutCallSite.ts
Lines 133 to 141 in bbc8bab
public get settings(): ISettings { | |
const myPrefix: unknown[] = | |
this._mySettings.prefix != null ? this._mySettings.prefix : []; | |
return { | |
...this._parentOrDefaultSettings, | |
...this._mySettings, | |
prefix: [...this._parentOrDefaultSettings.prefix, ...myPrefix], | |
}; | |
} |
Node.js Version
v16.16.0
Activity
terehov commentedon Aug 23, 2022
@dever23b Thank you for reporting this bug.
However, the faulty code isn't the spread operator, it's rather here:
tslog/src/LoggerWithoutCallSite.ts
Lines 170 to 176 in bbc8bab
It's the way the name gets replaced in certain scenarios. I have fixed it and going to publish a new version with some more bugfixes shortly.
Bugfix #146
terehov commentedon Aug 23, 2022
https://github.com/fullstack-build/tslog/releases/tag/v3.3.4
dever23b commentedon Aug 23, 2022
Thanks! I'll get updated and test this week.
dever23b commentedon Aug 25, 2022
I pulled the latest version and tested this. It works! Thanks again.