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

errWithCause only serializes Error objects #158

Open
darcyrush opened this issue Nov 12, 2024 · 4 comments
Open

errWithCause only serializes Error objects #158

darcyrush opened this issue Nov 12, 2024 · 4 comments

Comments

@darcyrush
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

darcyrush pushed a commit to darcyrush/pino-std-serializers that referenced this issue Nov 12, 2024
@darcyrush
Copy link
Author

Here is a PR with some questions as comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants