Skip to content

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

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

Closed
wants to merge 3 commits into from

Conversation

petamoriken
Copy link
Contributor

@petamoriken petamoriken commented May 3, 2025

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

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

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

@legendecas
Copy link
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.

Copy link

codecov bot commented May 12, 2025

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

It appears that we should wait for #57372.

@petamoriken petamoriken deleted the fix/dom-exception branch May 18, 2025 04:43
nodejs-github-bot pushed a commit that referenced this pull request Jun 21, 2025
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 <[email protected]>
    Commit-Queue: Chengzhong Wu <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: #58691
Fixes: #56497
Refs: #58138
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jun 21, 2025
Co-Authored-By: Kenta Moriuchi <[email protected]>
PR-URL: #58691
Fixes: #56497
Refs: v8/v8@e3df60f
Refs: #58138
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jun 23, 2025
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 <[email protected]>
    Commit-Queue: Chengzhong Wu <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#100894}

Refs: v8/v8@e3df60f
PR-URL: #58691
Fixes: #56497
Refs: #58138
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jun 23, 2025
Co-Authored-By: Kenta Moriuchi <[email protected]>
PR-URL: #58691
Fixes: #56497
Refs: v8/v8@e3df60f
Refs: #58138
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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.

DOMException should have [[ErrorData]] internal slot
5 participants