Skip to content
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

isError returns false for DOMException instances #2

Open
lionel-rowe opened this issue Nov 12, 2024 · 5 comments · May be fixed by #3
Open

isError returns false for DOMException instances #2

lionel-rowe opened this issue Nov 12, 2024 · 5 comments · May be fixed by #3

Comments

@lionel-rowe
Copy link

lionel-rowe commented Nov 12, 2024

isError returns false for DOMException instances.

From what I can see, there are three reasons for this:

if (isNativeError) { // node 10+
return isNativeError(arg);
}

In NodeJS, require('util').types.isNativeError(new DOMException()) gives false.

if ($structuredClone) {
try {
return $structuredClone(arg) instanceof $Error;

In current server-side implementations (pending whatwg/webidl#1421), structuredClone(new DOMException()) returns {}, i.e. a plain JS object.

if (!hasToStringTag || !(Symbol.toStringTag in arg)) {
var str = $toString(arg);
return str === '[object Error]' // errors
|| str === '[object DOMException]' // browsers

Even if this line was hit (which it won't be because the previous two lines would both return early with false), Symbol.toStringTag in new DOMException() returns true, so the condition fails.

It doesn't seem there's any 100% foolproof way of detecting DOMExceptions until whatwg/webidl#1421 lands in implementations, so you could either:

  • Be lax with Symbol.toStringTag tag specifically in the case of DOMException*, which would fail on DOMExceptions with spoofed toStringTag or other objects with toStringTag spoofed as "DOMException"
  • Bring forward instanceof checking specifically in the case of DOMException*, which would fail on cross-realm DOMExceptions. I guess this can't be tested for in Node as require('vm').runInNewContext('new DOMException()') gives DOMException is not defined, but such cross-realm DOMExceptions are presumably pretty commonplace in browsers (e.g. any DOMException originating from an iframe)

* and maybe DOMError and/or Exception too?

@ljharb
Copy link
Member

ljharb commented Nov 12, 2024

Given that it's bizarre to even have DOMException in node, and given that there's just no way to robustly detect it, I don't think that's a case worth worrying about (however, you may want to file it as a bug in node for util.types.isNativeError)

@lionel-rowe
Copy link
Author

it's bizarre to even have DOMException in node

IMO it's bizarre to have DOMException anywhere, it's just a badly designed API (badly named, weird semantics, weird behavior). But it's a commonly encountered error subtype, and no weirder that it exists in NodeJS than anywhere else. Bear in mind many DOMExceptions have nothing to do with the DOM, hence why it's badly named.

const ac = new AbortController()
ac.abort()
ac.signal.reason instanceof DOMException // true

@ljharb
Copy link
Member

ljharb commented Nov 12, 2024

yeah, that's part of the weirdness of AbortController, via fetch - it should never have been a DOMException.

Unless node offers a way to robustly identify it, though, I'm not sure if we should do anything for it.

@lionel-rowe
Copy link
Author

I don't think it's particularly to do with fetch, as AbortController and AbortSignal have other use cases too, e.g. canceling event listeners.

Other non-DOM-related and non-fetch-related APIs also produce DOMExceptions:

  • structuredClone({ fn: () => {} }) throws a DataCloneError DOMException
  • atob('!') throws an InvalidCharacterError DOMException
  • There are also various DOMException types produced from IndexedDB operations, although those are only relevant to browsers (but again, not DOM-related)

@ljharb
Copy link
Member

ljharb commented Nov 12, 2024

I'm just talking about the history - Abort things were built to provide cancellation to fetch, initially. That there are many other use cases is why using DOMException made no sense. Similarly, anything that's not DOM-related simply shouldn't be throwing an error named "DOM" ¯\_(ツ)_/¯ but that ship has sailed.

Regardless of what the behavior is here, I'd love to add test cases for all of those if you want to submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants