Skip to content

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