feat(serializers): Implemented "error with cause" serializer #130

Yuval-Peled · 2023-03-29T13:01:44Z

Created new serializer called "errWithCause". This serializer acts very similarly to the default "err" serializer. The differences: When it encounters an "error.cause" field, it serializes it recursively to create a nested object of errors. This is as opposed to the default error serializer, in which the resulting serialized object has no "cause" field, and the causes' messages & stack traces are appended to the original error.message and error.stack
Since some logic is reused between the existing err serializer and the new one, I moved some logic to the err-helpers file
Created a test-file for the new serializer. It is very similar to the existing err serializer test file since they should behave nearly identically. The differences are in the tests that test for the "cause" related behavior.
Upgrade tsconfig.json "lib" to support error.cause fields.

closes #126

Open questions for this PR:

I did not implement support for VError style "cause", which is a function. Should we add a check for this specific use case? My gut says no, since this serializer is meant for native Errors with "cause", so we should not strive to support each "extended error" library.
The test file I added has a lot of duplication w.r.t the default error serializer test. Would it be better to have a single test file for both so that tests that are relevant for both can be configured to run once for each serializer?
I bumped the tsconfig.json lib config to support new Error(message, options) (otherwise it only expects one argument). As far as I'm aware this does not break backwards compatibility since the target did not change - but I may be wrong here. Thoughts?
This PR should be accompanied by a PR to the pino documentation so that pino users are aware this new serializer exists

Thank you

- Created new serializer called "errWithCause". This serializer acts very similarly to the default "err" serializer. The differences: When it encounters an "error.cause" field, it serializes it recursively to create a nested object of errors. This is as opposed to the default error serializer, in which the resulting serialized object has no "cause" field, and the causes' messages & stack traces are appended to the original error.message and error.stack - Since some logic is reused between the existing err serializer and the new one, I moved some logic to the err-helpers file - Created a test-file for the new serializer. It is very similar to the existing err serializer test file since they should behave nearly identically. The differences are in the tests that test for the "cause" related behavior. - Upgrade tsconfig.json "lib" to support error.cause fields. pinojs#126

lib/err-helpers.js

Yuval-Peled · 2023-03-29T13:38:11Z

These tests fail for node v14. Working on a fix

kibertoad · 2023-03-29T13:46:32Z

tsconfig.json

@@ -1,7 +1,7 @@
 {
  "compilerOptions": {
    "target": "es6",
-    "lib": [ "es2015" ],
+    "lib": [ "es2022" ],


wouldn't that be a breaking change? what is the reason for it?

This is because I added a TS type test that creates an error with a cause, but in lib: es2015 this isn’t supported so the test fails to compile.

I was under the impression that the lib config does not impact the users of this library, only the developers of the library. Am I wrong? If so I will revert this change.

Just checked the TS config documentation. Seems that this config does not change the compilation result, so it does not affect library consumers. I think it's safe to change

What it does do is negate any alerts TS would give about using too new API syntax.

Could an alternative be to do eg. cause in err in the specific place instead?

Second the @voxpelli suggestion

test/err-with-cause.test.js

jsumners · 2023-03-29T14:38:31Z

I did not implement support for VError style "cause", which is a function. Should we add a check for this specific use case?

No. I have zero interest in supporting VError. Let's standardize on the native object.

The test file I added has a lot of duplication w.r.t the default error serializer test. Would it be better to have a single test file for both so that tests that are relevant for both can be configured to run once for each serializer?

Probably not, but I haven't actually clicked on the files tab yet. Personally, I don't like complicated test setups. I'd rather see all relevant details of the test inline with the test.

I bumped the tsconfig.json lib config to support new Error(message, options) (otherwise it only expects one argument). As far as I'm aware this does not break backwards compatibility since the target did not change - but I may be wrong here. Thoughts?

🤷‍♂️

This PR should be accompanied by a PR to the pino documentation so that pino users are aware this new serializer exists

That is reasonable. Such a PR should be a follow up after this is merged and released.

…r with 2nd argument

lib/err.js

test/err-with-cause.test.js

lib/err-helpers.js

lib/err-proto.js

@jsumners

According to code review by @jsumners Co-authored-by: James Sumners <[email protected]>

jsumners

I think the only thing missing at this point is the documentation.

Yuval-Peled · 2023-03-29T15:55:10Z

I think the only thing missing at this point is the documentation.

You're right - I missed that there's a readme.md in this repo as well (and not just in pino's original repo).
Will be done in a couple of minutes

Readme.md

Co-authored-by: James Sumners <[email protected]>

jsumners

Looks good to me.

jsumners · 2023-04-09T13:02:21Z

ping @mcollina

mcollina

lgtm

Yuval-Peled mentioned this pull request Mar 29, 2023

Discussion continuation: current design decision for error.cause is surprising and confusing #126

Closed

Yuval-Peled commented Mar 29, 2023

View reviewed changes

lib/err-helpers.js Outdated Show resolved Hide resolved

kibertoad reviewed Mar 29, 2023

View reviewed changes

Yuval-Peled commented Mar 29, 2023

View reviewed changes

test/err-with-cause.test.js Show resolved Hide resolved

fix test to work with node v14 that does not support Error constructo…

279678e

…r with 2nd argument

jsumners requested changes Mar 29, 2023

View reviewed changes

lib/err.js Outdated Show resolved Hide resolved

test/err-with-cause.test.js Show resolved Hide resolved

lib/err-helpers.js Outdated Show resolved Hide resolved

Code review fix: Moved error prototype related code to its own file

acaf194

jsumners reviewed Mar 29, 2023

View reviewed changes

lib/err-proto.js Outdated Show resolved Hide resolved

Add "use strict" and fix whitespace according to style

1256c27

According to code review by @jsumners Co-authored-by: James Sumners <[email protected]>

Yuval-Peled requested a review from jsumners March 29, 2023 15:33

jsumners requested changes Mar 29, 2023

View reviewed changes

Yuval-Peled added 2 commits March 29, 2023 19:15

Add documentation for errWithCause

c24daa9

Merge remote-tracking branch 'origin/master'

73738c8

Yuval-Peled requested a review from jsumners March 29, 2023 16:16

jsumners reviewed Mar 30, 2023

View reviewed changes

Readme.md Outdated Show resolved Hide resolved

Update Readme.md according to code review suggestion

2751985

Co-authored-by: James Sumners <[email protected]>

Yuval-Peled requested a review from jsumners April 3, 2023 08:25

jsumners approved these changes Apr 3, 2023

View reviewed changes

jsumners requested a review from mcollina April 3, 2023 14:44

mcollina approved these changes Apr 9, 2023

View reviewed changes

mcollina merged commit 0ccfa26 into pinojs:master Apr 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(serializers): Implemented "error with cause" serializer #130

feat(serializers): Implemented "error with cause" serializer #130

Yuval-Peled commented Mar 29, 2023

Yuval-Peled commented Mar 29, 2023

kibertoad Mar 29, 2023

Yuval-Peled Mar 29, 2023

Yuval-Peled Mar 29, 2023

voxpelli Apr 3, 2023

kibertoad Apr 3, 2023

jsumners commented Mar 29, 2023

jsumners left a comment

Yuval-Peled commented Mar 29, 2023

jsumners left a comment

jsumners commented Apr 9, 2023

mcollina left a comment

feat(serializers): Implemented "error with cause" serializer #130

feat(serializers): Implemented "error with cause" serializer #130

Conversation

Yuval-Peled commented Mar 29, 2023

Open questions for this PR:

Yuval-Peled commented Mar 29, 2023

kibertoad Mar 29, 2023

Choose a reason for hiding this comment

Yuval-Peled Mar 29, 2023

Choose a reason for hiding this comment

Yuval-Peled Mar 29, 2023

Choose a reason for hiding this comment

voxpelli Apr 3, 2023

Choose a reason for hiding this comment

kibertoad Apr 3, 2023

Choose a reason for hiding this comment

jsumners commented Mar 29, 2023

jsumners left a comment

Choose a reason for hiding this comment

Yuval-Peled commented Mar 29, 2023

jsumners left a comment

Choose a reason for hiding this comment

jsumners commented Apr 9, 2023

mcollina left a comment

Choose a reason for hiding this comment