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

add [[ErrorData]] slot to DOMExceptions #1421

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ljharb
Copy link
Contributor

@ljharb ljharb commented Jul 18, 2024

This is an attempt to fix tc39/proposal-is-error#9.

The intention is to make DOM Exceptions true subclasses of Error.

It's possible this PR needs additional changes to simplify/undo #732 - I'm happy to make whatever changes are appropriate.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno: …
    • Node.js: …
    • webidl2.js: …
    • widlparser: …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

This is part of a stage 2 TC39 proposal for Error.isError. When it advances to stage 2.7, I'll file the appropriate implementation bugs.


Preview | Diff

@ljharb
Copy link
Contributor Author

ljharb commented Aug 27, 2024

ping @domenic @annevk, this proposal is on the agenda for advancement next plenary, and i'd love to know that this direction is acceptable :-)

@domenic
Copy link
Member

domenic commented Aug 27, 2024

Seems fine directionally. In general we want DOMException to behave as if its constructor had a super() call and I think this aligns with that.

Editorially I wonder if putting it as a step in https://webidl.spec.whatwg.org/#internally-create-a-new-object-implementing-the-interface would be better. (E.g. modifying step 4.)

I also don't understand the relationship to #732 (serializability).

@ljharb
Copy link
Contributor Author

ljharb commented Aug 28, 2024

How would you suggest I modify step 4?

The relationship to #732, as I understand it (but may indeed understand incorrectly) is that that PR was only necessary because DOMExceptions aren't proper Errors - but by making them proper Errors, then serialization of DOMExceptions should be automatic now?

@domenic
Copy link
Member

domenic commented Aug 28, 2024

How would you suggest I modify step 4?

By adding [[ErrorData]] to the argument list of MakeBasicObject, if the inclusive inherited interfaces of interface includes DOMException.

but by making them proper Errors, then serialization of DOMExceptions should be automatic now?

No, it's not automatic, and it should not be.

The relevant step is step 17 of https://html.spec.whatwg.org/#structuredserializeinternal. Thankfully, it's guarded by "and value is not a platform object", which means DOMException will properly go down to step 19 instead.

If we took the generic step 17 path for DOMException, we would lose the name information, and would look at the public message property instead of the internal message.

@ljharb
Copy link
Contributor Author

ljharb commented Aug 28, 2024

ah, gotcha. ok cool, i'll make those changes to step 4, thanks!

@ljharb
Copy link
Contributor Author

ljharb commented Aug 28, 2024

hmm - looking at the MakeBasicObject call, that seems like it's for all platform objects, not just DOMExceptions. Would something in https://webidl.spec.whatwg.org/#js-creating-throwing-exceptions make more sense?

If the MakeBasicObject call is correct, then how would I modify that to only add the slot for DOMExceptions and not for other things?

@annevk
Copy link
Member

annevk commented Aug 28, 2024

I'd put the list passed to MakeBasicObject in a variable and then conditionally append [[ErrorData]] to it.

@ljharb
Copy link
Contributor Author

ljharb commented Aug 28, 2024

Thanks, updated! I left the original note I added as informative, but can remove it if preferred.

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

Successfully merging this pull request may close these issues.

HTML integration
3 participants