fix: support logging errors that do not contain a stack

[disintegrator]

Fixes #456

Custom errors may not have their stack property set.
Pino used to serialize undefined stack which created invalid JSON output

Given the following contrived example:

const logger = require('pino')();
const err = new Error();
delete err.stack;
logger.error(err);

Would output:

{"level":50,"time":1532087320790,"msg":"","pid":20631,"hostname":"dev.local","type":"Error","stack":undefined,"v":1}

This is invalid JSON due to the "stack":undefined at the end of that output line.

This PR adds a fix to avoid logging the stack if it is not set. So the above code now outputs:

{"level":50,"time":1532094347667,"msg":"","pid":25624,"hostname":"dev.local","type":"Error","v":1}

[mcollina]

LGTM

[davidmarkclements]

nice work

[jsumners]

Since null is valid JSON, wouldn't the more appropriate fix be to serialize it as null if error.stack === undefined?

[mcollina]

I’d prefer to keep it as a string, so that users would always decode a string. Errors should really have the stack property. Il giorno sab 21 lug 2018 alle 14:46 James Sumners <notifications@github.com> ha scritto:

…

Since null is valid JSON, wouldn't the more appropriate fix be to serialize it as null if error.stack === undefined? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#457 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL41FpcC7yy7QDx7jFQIHbfxT8B8owks5uIyKKgaJpZM4VYD76> .

[jsumners]

See #382

[davidmarkclements]

... I think we've made a mistake here.

For the current implementation, due to coercion, there are seven ways in which there may not be a stack

• delete
• null
• undefined value
• false
• empty string
• 0
• NaN

Let's remove the silly ones and go to the ones where a user may be attempting something real:

• delete
• null
• undefined value
• false
• empty string

What's the (likely) expectations in each case?

• delete - I want to remove stack. In this case there should be no stack property
• null - I want stack to be null (as per PrettyPrint fails when error's stack is null #382). In this case it should be null. That's a valid JSON property.
• undefined value - I want stack to have the value undefined. This can't be done with JSON, in this case only we might consider making it an empty string. But we could also just allow it to be removed and educate anyone who doesn't know this part of the JSON spec.
• false - I want to stack to be false. Then it should be false.
• empty string - I want stack to be an empty string. This will work anyway, but with extra unnecessary steps.

I should have spent more time reviewing this. I'm not sure what to do with this in v4, but in v5 we need to be more thoughtful in how we approach this.

[jsumners]

You understand the problem now :)

[github-actions]

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.