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

Allow DOMException subclasses to be used as exceptions #1211

Merged
merged 3 commits into from
Jun 5, 2023

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 8, 2022

See discussion in #1168 (comment).

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.

It seems some places were already doing this sort of thing, such as https://w3c.github.io/webtransport/#web-transport-error-interface and I think some media thing. For WebTransport, it seems like they aren't violating the letter of the current spec since they were never throwing such WebTransportErrors or treating them like exceptions, instead just passing them through streams machinery and so on. But I think it's a good idea to codify this.

Note that WebTransportError's constructor violates the proposal here of requiring such errors to always take a message as their first constructor argument. Not sure exactly what to do about that...

  • At least two implementers are interested (and none opposed):
    • Chrome has a spec being built that would directly use this.
    • I am hopeful other engines think this is a reasonable idea, e.g. based on the WebTransport precedent?
  • Tests are written and can be reviewed and commented upon at:
    • Nothing here is testable directly; we'd instead write tests for specific subclasses.
  • Implementation bugs are filed:
    • I don't think we need to preemptively do this; it would come along as part of the bugs for implementing such subclasses.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me introduce you to https://w3c.github.io/webrtc-pc/#dom-rtcerror which follows almost none of these guidelines and promises are rejected with instances of it already as far as I can tell.

index.bs Outdated Show resolved Hide resolved
"{{NotAllowedError}}" {{DOMException}} immediately after checking if the user has provided
permission to use a given feature, it's fairly obvious what sort of [=exception/message=] the
implementation should construct, and so specifying it is not necessary.
</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add here that "context" cannot be given in all situations and that user agents have to be careful not to introduce additional tracking vectors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more about ensuring that the message value which implementers construct from the context given by the spec author, does not introduce tracking vectors. I'll add it over in that section; let me know what you think.

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@domenic
Copy link
Member Author

domenic commented Oct 10, 2022

What should we do about the non-compliant existing cases? Realistically I doubt anyone will update them. So I think we could either soften all the recommendations to "should", or we could retro-classify them as willful violations.

@domenic
Copy link
Member Author

domenic commented Oct 10, 2022

The build says

IDL ERROR LINE: 3 - Required argument "options" cannot follow optional argument "message"

but I cannot actually find a requirement in the Web IDL spec corresponding to this... I thought you could do new WidgetError(undefined, opts) to trigger the default value for the message. Anyone able to enlighten me?

index.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Oct 10, 2022

Thank you for filing those downstream issues! I suggest that if they cannot become compliant we call them out explicitly as exceptions (hah!) not to be repeated.

@dandrader
Copy link

It's not clear to me whether a spec would be allowed to create a DOMException with a new error name (ie, not from that table) directly, without defining an interface that inherits from DOMException.

eg: Throw a "SomeSpecSpecificError" DOMException.
instead of: Throw a SpecError with some-error-code

@annevk
Copy link
Member

annevk commented Oct 10, 2022

So for WidgetErrorOptions one thing is that the argument needs = {} at the end. Perhaps that is enough to silence widlparser? I would be somewhat surprised if the required dictionary argument were to blame.

@domenic
Copy link
Member Author

domenic commented Oct 10, 2022

It's not clear to me whether a spec would be allowed to create a DOMException with a new error name (ie, not from that table) directly, without defining an interface that inherits from DOMException.

The intention is you are not supposed to do that. And you are not supposed to define subclasses, just because you want a new name. Instead you are supposed to come coordinate on the Web IDL repo to determine if any existing names will suit your purpose, and if not, send a PR to add it to the table.

I agree making this explicit is a good idea, and I'll do so.

@domenic
Copy link
Member Author

domenic commented Oct 10, 2022

So for WidgetErrorOptions one thing is that the argument needs = {} at the end. Perhaps that is enough to silence widlparser? I would be somewhat surprised if the required dictionary argument were to blame.

The WidgetErrorOptions are intentionally not optional, since they have a required member. So we can't default them, I don't think.

@dandrader
Copy link

It's not clear to me whether a spec would be allowed to create a DOMException with a new error name (ie, not from that table) directly, without defining an interface that inherits from DOMException.

The intention is you are not supposed to do that. And you are not supposed to define subclasses, just because you want a new name. Instead you are supposed to come coordinate on the Web IDL repo to determine if any existing names will suit your purpose, and if not, send a PR to add it to the table.

A spec will probably want error names that are rather specific to its problem space and thus not generally useful. I suppose the criteria for adding entries to this table is that the error name could be useful in several different contexts or problem domains? Would be good to clarify the criteria or rationale for adding new ones.

I also find it a bit confusing that a spec that defines a new interface inheriting from DOMException will also define a new, matching, error name, but that error name won't be present in that table. Or will it?

@annevk
Copy link
Member

annevk commented Oct 10, 2022

I see, I think there used to be a requirement that optional cannot be followed by non-optional, but I cannot find that now.

I also find it a bit confusing that a spec that defines a new interface inheriting from DOMException will also define a new, matching, error name, but that error name won't be present in that table. Or will it?

I think it would make sense to add these (as well as a reference to the class) to encourage reuse.

@domenic
Copy link
Member Author

domenic commented Oct 20, 2022

I've updated this to reflect all of the discussion so far. It is blocked by plinss/widlparser#79, but otherwise, I think, ready for merge.

Some things which we could change in followups:

  • I have not suggested that DOMException derived classes register themselves with Web IDL, via the error names table or otherwise. I think this is reasonable, similar to how we don't ask that interfaces in general register themselves with Web IDL.

  • I have encoded what I believe to be the current understanding of the DOMException names table, which is that specs must not use other names and should request additions to the table if they want to go beyond it. However, What to do about specs that need lots of error types #1219 investigates changing that.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a couple nits, looks great overall.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
kainino0x added a commit to kainino0x/gpuweb that referenced this pull request Nov 22, 2022
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
dandrader added a commit to WICG/web-smart-card that referenced this pull request Dec 28, 2022
kainino0x added a commit to gpuweb/gpuweb that referenced this pull request Dec 30, 2022
* Reject create*PipelineAsync on failure

Proposal

Fixes 3296

* Write more text about immediate vs promised pipeline creation

* add an issue/FIXME

* Extend DOMException, approximately according to whatwg/webidl#1211

* define GPUPipelineError.reason

* fix nit

* address comments

* Add issue about constructor "message" arg
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
@domenic domenic force-pushed the domexception-derived branch from fa75be9 to 05a972e Compare June 1, 2023 09:39
@domenic domenic force-pushed the domexception-derived branch from 05a972e to 50cf0cb Compare June 1, 2023 09:44
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
@domenic domenic force-pushed the domexception-derived branch from 50cf0cb to 74dae7b Compare June 1, 2023 09:51
@domenic
Copy link
Member Author

domenic commented Jun 1, 2023

Hey, the blocking build issue was fixed!! This is ready for review, and hopefully merging.

its corresponding error object in the
ECMAScript specification.
These correspond to all of the ECMAScript [=ECMAScript/error objects=] (apart from
<l spec=ecmascript>{{SyntaxError}}</l> and <l spec=ecmascript>{{Error}}</l>, which are deliberately
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-existing, but: AggregateError, too.

(Given there’s at least one more in the pipeline, maybe keeping this in sync is too awkward and the gist could be conveyed without enumerating the excluded constructors...?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either file a follow-up or address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk distinct issue here: #1314

index.bs Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo nits and question.

its corresponding error object in the
ECMAScript specification.
These correspond to all of the ECMAScript [=ECMAScript/error objects=] (apart from
<l spec=ecmascript>{{SyntaxError}}</l> and <l spec=ecmascript>{{Error}}</l>, which are deliberately
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either file a follow-up or address.

index.bs Outdated Show resolved Hide resolved
* Their [=constructor operation=] must take as its first parameter an [=optional argument|optional=]
{{DOMString}} named |message| defaulting to the empty string, and must set the instance's
[=DOMException/message=] to |message|.
* Their [=constructor operation=] should take as its second parameter a [=dictionary=] containing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the switch from must to should intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm not 100% sure, but I feel like being too prescriptive here is not necessary, since we don't know exactly what shape the additional information will come in.

Co-authored-by: Anne van Kesteren <[email protected]>
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.

6 participants