FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add a `[[ErrorData]]` internal slot to DOMException by petamoriken · Pull Request #58138 · nodejs/node · GitHub

/ node Public

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

Closed
petamoriken wants to merge 3 commits into
nodejs:mainfrom
petamoriken:fix/dom-exception
ConversationCommits (3)ChecksFiles changed
Closed

Add a [[ErrorData]] internal slot to DOMException#58138
petamoriken wants to merge 3 commits into
nodejs:mainfrom
petamoriken:fix/dom-exception

Conversation

petamoriken commented May 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #56497

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

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

bnoordhuis commented May 4, 2025

Copy link
Copy Markdown
Member

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 commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

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

legendecas commented May 12, 2025

Copy link
Copy Markdown
Member

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 Bot commented May 12, 2025 •
edited
Loading

Copy link
Copy Markdown

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 commented May 15, 2025

Copy link
Copy Markdown
Contributor Author

It appears that we should wait for #57372.

petamoriken deleted the fix/dom-exception branch May 18, 2025 04:43
legendecas mentioned this pull request Jun 12, 2025
Closed
nodejs-github-bot pushed a commit that referenced this pull request Jun 21, 2025
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
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
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
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 21, 2025
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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 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

Back | FazBrowse Home | New Git URL