-
Notifications
You must be signed in to change notification settings - Fork 875
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
Fixed stream metadata loss during prettification #780
Conversation
For streams with Symbol('metadata') true flag, pino metadata was lost during prettification
lib/tools.js
Outdated
flushSync () { | ||
if (warned) { | ||
return | ||
} | ||
warned = true | ||
dest.write(pretty(Object.assign({ | ||
dest.write.call(this.destStreamContext, pretty(Object.assign({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying streams metadata might cause significant issues. I would just attach the properties to the stream itself, mutating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do something like:
function setMetadataProps (dest, that) {
if (dest[needsMetadataGsym] === true) {
dest.lastLevel = that.lastLevel
dest.lastMsg = that.lastMsg
dest.lastObj = that.lastObj
dest.lastTime = that.lastTime
dest.lastLogger = that.lastLogger
}
}
and then call in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created setMetadataProps function
…eam instance if need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
For streams with Symbol('metadata') true flag, pino metadata was lost during prettification.
RE: pinojs/pino-multi-stream#31