Repository navigation
Conversation
|
Can you rebase? This is now conflicting. |
| test('serializes SuppressedError error and suppressed', { skip: !global.SuppressedError }, () => { | ||
| const error = new Error('the operation failed') | ||
| const suppressed = new Error('disposal also failed') | ||
| const err = new global.SuppressedError(error, suppressed, 'an error was suppressed during disposal') |
There was a problem hiding this comment.
As I linked in the original issue: this error object is 1. not actually a standard yet and 2. is not available until Node.js v24.11.1.
There was a problem hiding this comment.
Thanks for raising the Node-version concern. The new tests no longer skip this behavior on Node 18 or 20: they use matching non-enumerable property descriptors there, and the native constructor when available. I ran all 13 new regressions under Node 18.20.8, 20.20.2 and 24.11.1; all passed. The implementation only reads the properties and does not require a global SuppressedError constructor.
72203e5 to
3978b40
Compare
|
Rebased onto current master. I replaced the skipped test with coverage that runs on Node 18 and 20 as well as native |
| // SuppressedError stores these properties as non-enumerable own properties. | ||
| for (const key of ['error', 'suppressed']) { | ||
| if (!Object.prototype.hasOwnProperty.call(err, key) || Object.prototype.propertyIsEnumerable.call(err, key)) { | ||
| continue | ||
| } | ||
| const val = err[key] | ||
| if (isErrorLike(val)) { | ||
| if (!Object.prototype.hasOwnProperty.call(val, seen)) { | ||
| _err[key] = errSerializer(val) | ||
| } | ||
| } else { | ||
| _err[key] = val | ||
| } | ||
| } |
There was a problem hiding this comment.
The lines between 64 and 76, on cursory review, look like duplicated lines but there is a subtle difference between them: the cause key is skipped in a without cause serialization. So the blocks have to be maintained indepdendently.
The same cannot be said for the lines between 78 and 91. In both cases these lines are identical. They should really call out to a common method:
maybeSerializeSuppressedError(error, target) {
/* loop from 78 - 91 */
}There was a problem hiding this comment.
Moved the common handling into maybeSerializeSuppressedError, with each serializer passing its own recursive callback. I added coverage for nested causes so the two serializers keep their different cause behavior. Lint, type checks and all 88 tests pass; the 15 suppressed-error regressions also pass on Node 18, 20 and 24.
|
Thanks for the review. The latest CI checks are green, and the serializer and type coverage requested in the review is included. Is there anything else you would like me to address before this can be merged? |
Preserve the non-enumerable
errorandsuppressedproperties ofSuppressedErrorin both error serializers. A shared helper handles both serializers; nested error-like values use each serializer’s existing cause handling and cycle guard; non-error values are retained. Theerr()serializer still leaves customtoJSON()output in control.Added regressions for nested and circular errors, shared references, repeated serialization, non-error values and
toJSON(). Tests use nativeSuppressedErrorwhen available and matching property descriptors on older Node versions.Validation:
npm run lint-ci,npm run test-typesandnpm run test-cipassed (88 tests). The 15 suppressed-error regressions also passed under Node 18.20.8, 20.20.2 and 24.11.1.Closes #199