Skip to content

Conversation

@Renegade334
Copy link
Contributor

Currently, errors deserialised by error_serdes are recreated using Object.create() with the source error's prototype. The objects created using this technique are not considered native errors.

This was always observable using util.types.isNativeError(), but with Error.isError() now being exposed, this becomes more readily apparent.

Closes #48716.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 6, 2025
]);

match(stderr, /SyntaxError \[Error\]:/);
match(stderr, /SyntaxError:/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this previous behaviour was a result of always assigning [Symbol.toStringTag]: 'Error' on the deserialised error.

@Renegade334 Renegade334 force-pushed the serdes-deserialize-to-native-error branch 2 times, most recently from e919a34 to b353e31 Compare June 6, 2025 17:18
validateError(error, Error);
assert.notStrictEqual(error.cause, undefined);
validateError(error.cause, TypeError);
}));
Copy link
Member

Choose a reason for hiding this comment

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

Would be worth expanding this test to see if it works with the new SuppressedError and AggregateError types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider AggregateError, although neither of these are explicitly handled currently.

const errors = {
Error, TypeError, RangeError, URIError, SyntaxError, ReferenceError, EvalError,
};

At the moment, the logic traverses the prototype chain and ends up serialising these as Errors, so they end up with a prototype of Error.prototype. It doesn't really fall within the scope of this PR, but I can add a todo if desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a TODO to error_serdes for the uncovered constructors.

@codecov
Copy link

codecov bot commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.12%. Comparing base (8d11399) to head (71f1340).

Files with missing lines Patch % Lines
lib/internal/error_serdes.js 93.33% 1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## main #58605 +/- ## ======================================= Coverage 90.11% 90.12% ======================================= Files 640 640 Lines 188427 188432 +5 Branches 36965 36961 -4 ======================================= + Hits 169804 169816 +12  + Misses 11347 11337 -10  - Partials 7276 7279 +3 
Files with missing lines Coverage Δ
lib/internal/error_serdes.js 98.55% <93.33%> (+0.03%) ⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
@Renegade334 Renegade334 force-pushed the serdes-deserialize-to-native-error branch from b353e31 to 71f1340 Compare July 1, 2025 22:38
@Renegade334 Renegade334 requested review from anonrig and jasnell July 23, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run.

4 participants