Skip to content

lib,src: preserve domexception in v8 serialize/deserialize#61856

Open
efekrskl wants to merge 3 commits intonodejs:mainfrom
efekrskl:fix/v8-serialize-deserialize-domexception
Open

lib,src: preserve domexception in v8 serialize/deserialize#61856
efekrskl wants to merge 3 commits intonodejs:mainfrom
efekrskl:fix/v8-serialize-deserialize-domexception

Conversation

@efekrskl
Copy link
Contributor

Closes #53225

Not sure whether v8.serialize()/deserialize() should preserve DOMException, but I took a shot since structuredClone() already does. This should treat DOMExceptions as a host object and use a escape hatch for serializing them on our end.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 module Issues and PRs related to the "v8" subsystem. labels Feb 16, 2026
@codecov
Copy link

codecov bot commented Feb 16, 2026

Codecov Report

❌ Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.72%. Comparing base (4f13746) to head (b033bf4).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/node_serdes.cc 72.22% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #61856   +/-   ##
=======================================
  Coverage   89.72%   89.72%           
=======================================
  Files         675      675           
  Lines      204806   204849   +43     
  Branches    39355    39363    +8     
=======================================
+ Hits       183761   183807   +46     
- Misses      13330    13331    +1     
+ Partials     7715     7711    -4     
Files with missing lines Coverage Δ
lib/v8.js 99.08% <100.00%> (+0.04%) ⬆️
src/node_serdes.cc 68.75% <72.22%> (-0.13%) ⬇️

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

No particularly strong feelings, but I'll point out that the ability to (de)serialize custom objects is part of why Serializer and Deserializer are exposed as abstract classes

*/
_writeHostObject(abView) {
// Route DOMException through the same hooks used by structuredClone
if (abView instanceof DOMException) {
Copy link
Member

Choose a reason for hiding this comment

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

DOMException is a per-context concept in Node.js, so I don't think instanceof is a reliable brand check (as usual in JS)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 module Issues and PRs related to the "v8" subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOMExceptions do not work with v8.serialize()

3 participants