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

Do not cast to string Error#stack. #464

Merged
merged 1 commit into from
Jul 29, 2018
Merged

Conversation

mcollina
Copy link
Member

@davidmarkclements
Copy link
Member

yes I agree - but this is now a breaking change, so should be in v5 but not v4

@davidmarkclements
Copy link
Member

We also need to discuss one case:

If I delete err.stack then "stack" should not show.

However if I assign: err.stack = undefined, then some make the argument that they'd like to see, in some way, that it's been set to undefined. Which is how we got here.

So, should we set stack to an empty string when the stack is present but the value is undefined?

@mcollina
Copy link
Member Author

yes I agree - but this is now a breaking change, so should be in v5 but not v4

I don't think so. We landed a botched commit that we shouldn't. Let's restore proper behavior.

However if I assign: err.stack = undefined, then some make the argument that they'd like to see, in some way, that it's been set to undefined. Which is how we got here.

No, you do not want to see it. Mainly because:

pino.info({ hello: undefined })

Will not show the 'hello' property. undefined is invisible in JSON, and it is equal to delete.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

@jsumners
Copy link
Member

I agree with Matteo, this is not a major but is really a fix to restore behavior.

Does this pass the awesome test suite David built?

@mcollina
Copy link
Member Author

I do not know how to run that, and there are no instructions. However it does fix the pino-pretty test, so I think we are good to go.

@mcollina mcollina merged commit 576c970 into master Jul 29, 2018
@mcollina mcollina deleted the empty-stack-no-string-casting branch July 29, 2018 16:48
@davidmarkclements
Copy link
Member

To run just go to travis and restart build for master - prior to release. We don't have a way to run for pr's atm

@github-actions
Copy link

github-actions bot commented Feb 6, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests failing in v4.17.5
3 participants