Serialize errors with cause

[hypesystem]

Related to #76 and #78 which add serialization of cause into error messages.

This PR fixes #94 by making sure the chain of causes found in err.cause is serialized in full, bringing the final serialized result closer to what is printed by other serializers (e.g. those in the Node.js CLI and browser consoles).

[mcollina]

lgtm

[mcollina]

@voxpelli could you take a look?

[mcollina]

CI is failing

[hypesystem]

It seems that my assumption (comment) "message serialization already walks cause-chain" is only true in Node.js 16 for some reason? (I am running 16 locally, too, so it might be...) https://github.com/pinojs/pino-std-serializers/pull/105/files#diff-a6ff895a4ce70729791324c8118f43cffada5a9e9cfe42ea1df4a18707f2c430R207

This is odd to me, because it looks like the same functionality should be happening (cf. what was done in #76). Could it be that earlier Node.js versions don't support the cause key in options?

[hypesystem]

Oh yes, looking at it now what fails is the following lines:

t.equal(serialized.message, 'bar: foo') // message serialization already walks cause-chain
t.ok(serialized.cause)
t.equal(serialized.cause.type, 'Error')

I'll try to look at other tests to see if similar things are handled there.

[hypesystem]

In the other tests (of cause serialization into the error message) the .cause field is set manually on the Error, instead of using the options object: https://github.com/pinojs/pino-std-serializers/blob/master/test/err.test.js#L51-L53

This could be an approach, of course, but it kind of obscures what the case we are trying to support is... Open to input on how to approach this 😄

[mcollina]

That's totally ok, follow the other tests.

[hypesystem]

Fixed 😄

[hypesystem]

Hmm this comment on recursion by @jsumners made me realize that in #78 there was added some more checks, so it only recurses if _err.cause is an Error, and also has support for traversing the case where _err.cause is a function (for VError, which I'm not quite sure what is). C.f. https://github.com/pinojs/pino-std-serializers/blob/master/lib/err-helpers.js#L20-L31

Should I support that here too?

[jsumners]

Looks good to me.

[voxpelli]

Um, so this now means that apart from the .stack getting all of the cause stacks appended and .message getting all of the cause messages appended we're now also doing that for each cause in itself?

So we're basically having two systems for serializing the causes at once?

I would say that either we walk the cause chain as this PR did, or we summarize the cause chain through the .stack and the .message as I did in #78.

Never do the both at once and especially never do the both recursively?

Sorry for not responding earlier :/

[hypesystem]

@voxpelli I agree, and raised the issue here, too: #94 (comment)

[voxpelli]

And in https://github.com/pinojs/pino-std-serializers/pull/78/files#diff-71c187d84f435004e68161a25e470d823f4de310a1d39b531cc02a90f3730149R56 we specifically opted out causes from recursion:

if (val instanceof Error && key !== 'cause') {
  /* eslint-disable no-prototype-builtins */
  if (!val.hasOwnProperty(seen)) {

I think it should be rewritten as:

if (val instanceof Error) {
  /* eslint-disable no-prototype-builtins */
  if (key !== 'cause' && !val.hasOwnProperty(seen)) {

That way it will exclude all Error causes from serialization, as intended, but keeping other causes

[hypesystem]

I'm not sure I understand the explicit opting out of recursing over Errors? Could you elaborate on that @voxpelli

[voxpelli]

Let's discuss it in #110