Explicitly check cause property on Error instances

[ghost]

The current error serializer only walks enumerable properties using a for..in loop. The downside to this is that vanilla errors with their cause property set are not enumerable by default. I don't know if it's spec related or just specific to V8's implementation, but it's a bit wonky/unexpected nonetheless.

This means if you have the following code, the cause property won't actually be logged/printed. It will only log the very first/top-level error.

fastify.get('/throw', async (req, res) => {
  throw new Error('first cool error', {
    cause: new Error('second boring error', {
      cause: new Error('third awesome error'),
    }),
  });
});

Here's what node's inspector prints:

Uncaught [Error: first cool error] {
  [cause]: Error: second boring error
      at REPL5:2:12
      at Script.runInThisContext (node:vm:129:12)
      ... 7 lines matching cause stack trace ...
      at REPLServer.[_line] [as _line] (node:internal/readline/interface:879:18) {
    [cause]: Error: third awesome error
        at REPL5:3:14
        at Script.runInThisContext (node:vm:129:12)
        at REPLServer.defaultEval (node:repl:566:29)
        at bound (node:domain:421:15)
        at REPLServer.runBound [as eval] (node:domain:432:12)
        at REPLServer.onLine (node:repl:893:10)
        at REPLServer.emit (node:events:539:35)
        at REPLServer.emit (node:domain:475:12)
        at REPLServer.[_onLine] [as _onLine] (node:internal/readline/interface:418:12)
        at REPLServer.[_line] [as _line] (node:internal/readline/interface:879:18)
  }
}

Would people be open to a PR that explicitly checks for a cause key alongside the existing for..in check? Are there other ways to achieve this? It seems like this would be an expected/sane default, but perhaps there are downsides to doing this or was previously not done intentionally?

Thanks.

[mcollina]

Go for it, would you like to send a Pull Request to address this issue? Remember to add unit tests.

[ghost]

Yeah, I should be able to take a stab at it. I'm slightly confused actually, at first glance, it looks like the cause should be getting included in the message/stack based on https://github.com/pinojs/pino-std-serializers/blob/master/lib/err.js#L56-L57, which uses the error helpers, and since those don't depend on enumerable properties, not sure why they're actually missing all together. With that being said, I only looked through it at a glance, will explore a bit more this week if I can get some time.

[jsumners]

The first step is to write a failing test.

[hypesystem]

Is there any news on this? Otherwise I might try and do it

[kibertoad]

@hypesystem go for it!

[hypesystem]

Having almost finished writing the code for this, I noticed that .cause is currently serialized in the error message (see #76 and implementation in #78).

I think these two things are ... kind of redundant when they both exist?

Is there a use case for wanting both the error message to be constructed from .cause chains and to include the chain itself in the serialized result?

Personally, I would prefer the serialized result, as it would contain stack traces of the original Errors, but I'd like some input on how to progress here. What are your opinions?

[hypesystem]

I've added a PR on this #105, but pending my previous questions, I'm wondering if we should clean up the message serialization in the same PR.

[voxpelli]

Personally, I would prefer the serialized result, as it would contain stack traces of the original Errors

They are also preserved:

pino-std-serializers/lib/err.js

Line 62 in d8ce240

_err.stack = stackWithCauses(err)