Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Add a [[ErrorData]] internal slot to DOMException#58138

Closed
petamoriken wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom
petamoriken:fix/dom-exceptionpetamoriken/node:fix/dom-exceptionCopy head branch name to clipboard
Closed

Add a [[ErrorData]] internal slot to DOMException#58138
petamoriken wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom
petamoriken:fix/dom-exceptionpetamoriken/node:fix/dom-exceptionCopy head branch name to clipboard

Conversation

@petamoriken
Copy link
Contributor

@petamoriken petamoriken commented May 3, 2025 •
edited
Loading

Fixes #56497

Error.isError, implemented in V8 13.6 (#58070), must return true for DOMException.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 3, 2025
ExE-Boss
ExE-Boss reviewed May 3, 2025
View reviewed changes
lib/internal/per_context/domexception.js Outdated Show resolved Hide resolved
@petamoriken petamoriken force-pushed the fix/dom-exception branch from 66141cf to 75b30c2 Compare May 3, 2025 13:49
@petamoriken petamoriken changed the title fix: add a [[ErrorData]] internal slot to DOMException Add a [[ErrorData]] internal slot to DOMException May 3, 2025
@petamoriken petamoriken force-pushed the fix/dom-exception branch from 75b30c2 to 865c576 Compare May 3, 2025 14:50
@bnoordhuis
Copy link
Member

bnoordhuis commented May 4, 2025

Maybe a better way to fix this is to call isolate->SetJSApiWrapperNativeErrorCallback(IsNodeError) with IsNodeError looking something like this:

bool IsNodeError(Isolate* isolate, Local<Object> obj) {
  return IsDOMException(isolate, obj); // implementation of IsDOMException left as an exercise to the reader
}

Probably means implementing DOMException as an ObjectTemplate/FunctionTemplate in C++ land.

@petamoriken
Copy link
Contributor Author

petamoriken commented May 6, 2025

IMO, this PR should be merged before Node.js v24 is released. How about that?

@legendecas
Copy link
Member

legendecas commented May 12, 2025

Like @bnoordhuis suggested, for IsDOMException, we can use the private symbol is_dom_exception introduced in #57372 to fast check if an object is a dom exception.

@codecov
Copy link

codecov bot commented May 12, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (5d3e1b5) to head (d342ccd).
Report is 112 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58138      +/-   ##
==========================================
+ Coverage   89.64%   90.10%   +0.45%     
==========================================
  Files         630      630              
  Lines      186470   186623     +153     
  Branches    36305    36628     +323     
==========================================
+ Hits       167160   168151     +991     
+ Misses      12073    11252     -821     
+ Partials     7237     7220      -17
Files with missing lines Coverage Δ
lib/internal/per_context/domexception.js 77.57% <100.00%> (+1.75%) ⬆️

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

@petamoriken
Copy link
Contributor Author

petamoriken commented May 15, 2025

It appears that we should wait for #57372.

@petamoriken petamoriken deleted the fix/dom-exception branch May 18, 2025 04:43
@legendecas legendecas mentioned this pull request Jun 12, 2025
Closed
nodejs-github-bot pushed a commit that referenced this pull request Jun 21, 2025
@legendecas @nodejs-github-bot
deps: V8: cherry-pick e3df60f3f5ab
c1c3673 
Original commit message:

    [objects] allow host defined serializer of JSError

    Allow host defined serializer and deserializer of JSError in
    ValueSerializer API. This allows hosts that implement DOMException
    in JS to support `Error.isError` proposal and `structuredClone`.

    Refs: #58691
    Change-Id: I022821c9abd659970c4d449b3c69c5fb54d0618a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6637876
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: #58691
Fixes: #56497
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jun 21, 2025
@legendecas @petamoriken @nodejs-github-bot
lib: make domexception a native error
e679e38 
Co-Authored-By: Kenta Moriuchi <moriken@kimamass.com>
PR-URL: #58691
Fixes: #56497
Refs: v8/v8@e3df60f
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jun 23, 2025
@legendecas @RafaelGSS
deps: V8: cherry-pick e3df60f3f5ab
4b61f10 
Original commit message:

    [objects] allow host defined serializer of JSError

    Allow host defined serializer and deserializer of JSError in
    ValueSerializer API. This allows hosts that implement DOMException
    in JS to support `Error.isError` proposal and `structuredClone`.

    Refs: #58691
    Change-Id: I022821c9abd659970c4d449b3c69c5fb54d0618a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6637876
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: #58691
Fixes: #56497
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jun 23, 2025
@legendecas @petamoriken @RafaelGSS
lib: make domexception a native error
081c708 
Co-Authored-By: Kenta Moriuchi <moriken@kimamass.com>
PR-URL: #58691
Fixes: #56497
Refs: v8/v8@e3df60f
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
Renegade334 pushed a commit to Renegade334/node that referenced this pull request Sep 20, 2025
@legendecas @petamoriken @Renegade334
lib: make domexception a native error
956ad77 
Co-Authored-By: Kenta Moriuchi <moriken@kimamass.com>
PR-URL: nodejs#58691
Fixes: nodejs#56497
Refs: v8/v8@e3df60f
Refs: nodejs#58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
Renegade334 pushed a commit to Renegade334/node that referenced this pull request Sep 21, 2025
@legendecas @Renegade334
deps: V8: cherry-pick e3df60f3f5ab
2ec88e0 
Original commit message:

    [objects] allow host defined serializer of JSError

    Allow host defined serializer and deserializer of JSError in
    ValueSerializer API. This allows hosts that implement DOMException
    in JS to support `Error.isError` proposal and `structuredClone`.

    Refs: nodejs#58691
    Change-Id: I022821c9abd659970c4d449b3c69c5fb54d0618a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6637876
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: nodejs#58691
Backport-PR-URL: nodejs#59957
Fixes: nodejs#56497
Refs: nodejs#58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
Renegade334 pushed a commit to Renegade334/node that referenced this pull request Sep 21, 2025
@legendecas @petamoriken @Renegade334
lib: make domexception a native error
f8d1357 
Co-Authored-By: Kenta Moriuchi <moriken@kimamass.com>
PR-URL: nodejs#58691
Backport-PR-URL: nodejs#59957
Fixes: nodejs#56497
Refs: v8/v8@e3df60f
Refs: nodejs#58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Sep 22, 2025
@legendecas @richardlau
deps: V8: cherry-pick e3df60f3f5ab
d289b1d 
Original commit message:

    [objects] allow host defined serializer of JSError

    Allow host defined serializer and deserializer of JSError in
    ValueSerializer API. This allows hosts that implement DOMException
    in JS to support `Error.isError` proposal and `structuredClone`.

    Refs: #58691
    Change-Id: I022821c9abd659970c4d449b3c69c5fb54d0618a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6637876
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Chengzhong Wu <cwu631@bloomberg.net>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: #58691
Backport-PR-URL: #59957
Fixes: #56497
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Sep 22, 2025
@legendecas @petamoriken @richardlau
lib: make domexception a native error
edb9248 
Co-Authored-By: Kenta Moriuchi <moriken@kimamass.com>
PR-URL: #58691
Backport-PR-URL: #59957
Fixes: #56497
Refs: v8/v8@e3df60f
Refs: #58138
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ExE-Boss ExE-Boss ExE-Boss left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DOMException should have [[ErrorData]] internal slot

5 participants

@petamoriken @bnoordhuis @legendecas @ExE-Boss @nodejs-github-bot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.