Skip to content

lib: deserialize to native errors in error_serdes #58605

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors emitted on worker are not native errors
4 participants