errWithCause only serializes Error objects

[darcyrush]

Javascript can throw anything, not just Error objects. This includes primitives like strings and numbers;

try {
  throw 42; // completely valid
} catch (e) {
  throw new Error("Nested error", { cause: e });
}

The bundled lib.es2022.error.d.ts TypeScript typings indirectly hints at this;

interface Error {
    cause?: unknown;
}

As such, I believe this is unexpected;

const serialize = require("pino-std-serializers").errWithCause;
const serialized = serialize(new Error("Example", { cause: 42 }));
console.log(serialized.cause); // undefined

I would expect the cause to be populated with 42. Likewise I would expect it to be populated if a string was thrown, or whatever else is legal to be thrown in JavaScript, including objects that don't extend the JS Error class. Instead, the cause is simply lost.

This example may seem like a contrived edge case, but I have come across libraries that throw their own "Error" objects, that do not extend from the native JS Error class,

[jsumners]

Javascript can throw anything, not just Error objects. This includes primitives like strings and numbers;

That is true, but you should NOT do that. You should only throw instances of things that derive from Error.prototype. This library is designed with this principle in mind.

[darcyrush]

That is true, but you should NOT do that. You should only throw instances of things that derive from Error.prototype.

I completely agree, but best practices do not always translate to reality.

While we can enforce this in our own code, we have no way to enforce this in external npm libraries (or even in very old nodejs core code, potentially). Wrapping every single npm library in a try catch is not simply not feasible. The majority of errors that our projects deal with are thrown from npm libraries.

Given that JS allows this - regardless of how insane it is - i think pino serialization should account for this also.

[jsumners]

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

[darcyrush]

Here is a PR with some questions as comments

[ifeltsweet]

We also need this because we do something like this:

throw new Error("Request failed", { cause:  { request } }); 

Which is completely valid and useful.

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

[darcyrush]

We also need this because we do something like this:

throw new Error("Request failed", { cause:  { request } }); 

This is our use case too.

[voxpelli]

@ifeltsweet @darcyrush The preferable way to do that would be to make a RequestError or similar that has a request as a property

Then you can log it in a more desirable way

And strictly speaking the request is probably not the cause of your failure, something in your code is, the request is merely context.

If it's a HTTP code 400 style failure then the cause is that the request didn't pass validation or similar – the cause isn't the request itself.

If it's a HTTP code 500 style failure then the cause is that the request unexpectedly made something error in your code – then the cause is that error, not the request.

Adding the request to the Error may often be redundant as well – your pino setup in your router should annotate your logs with the request context by itself. Eg. Fastify does this when you use req.log and when it catches a thrown error (and a thrown error will always cause a 500 response unless the error is something like a NotFoundError)

[voxpelli]

For historical context:

The main reason why Error causes can be any value is because all of throw, Promise.reject(), the catch in a try-catch etc historically can be of any value.

So locking it down to be only Error:s that were allowed would have necessitated people to check whether it's an Error before they add it as a cause.

See original discussion at tc39/proposal-error-cause#21 and also see es-shims/error-cause#2

[lake2]

We also need this because we do something like this:

throw new Error("Request failed", { cause:  { request } }); 

This is our use case too.

same use case +1. It seems that not support yet?