-
Notifications
You must be signed in to change notification settings - Fork 165
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
Conversation
There was a problem hiding this 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.
"{{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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
The build says
but I cannot actually find a requirement in the Web IDL spec corresponding to this... I thought you could do |
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. |
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. |
So for |
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. |
The WidgetErrorOptions are intentionally not optional, since they have a required member. So we can't default them, I don't think. |
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? |
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 think it would make sense to add these (as well as a reference to the class) to encourage reuse. |
6242377
to
936dfd4
Compare
3145082
to
15b1350
Compare
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:
|
There was a problem hiding this 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.
* 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
fa75be9
to
05a972e
Compare
05a972e
to
50cf0cb
Compare
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.
50cf0cb
to
74dae7b
Compare
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 |
There was a problem hiding this comment.
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...?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
* 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]>
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
WebTransportError
s 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...(See WHATWG Working Mode: Changes for more details.)
Preview | Diff