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

Nested VError causes #108

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Conversation

hypesystem
Copy link
Contributor

It was bugging me that I had left out support for VError-style errors that are supported elsewhere when I made #105, so here's a PR adding support for that 😄

Extracts evaluateCause from inside err-helpers and reuses it in error serialization to support nested causes where .cause is a function.
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina requested a review from jsumners June 10, 2022 21:48
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@mcollina mcollina merged commit 5b88f7e into pinojs:master Jun 10, 2022
voxpelli added a commit to voxpelli/pino-std-serializers that referenced this pull request Jun 13, 2022
voxpelli added a commit to voxpelli/pino-std-serializers that referenced this pull request Jun 13, 2022
Instead rely on its content being appended to the message and the stack.

This is an alternative fix to pinojs#94, replacing pinojs#105 and pinojs#108 + makes pinojs#109 not needed
mcollina pushed a commit that referenced this pull request Jun 13, 2022
…ead (#110)

* Revert "Nested VError causes (#108)"

This reverts commit 5b88f7e.

* Revert "Serialize errors with cause (#105)"

This reverts commit ae83956.

* Never serialize `.cause` when an `Error`

Instead rely on its content being appended to the message and the stack.

This is an alternative fix to #94, replacing #105 and #108 + makes #109 not needed

* Add tests

* Add explainer comment
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

Successfully merging this pull request may close these issues.

3 participants