Skip to content

Conversation

@Renegade334
Copy link
Member

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
Member 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
Member 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
Member 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Errors emitted on worker are not native errors

4 participants