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

Normative: Add DOMException cause #1179

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link

@legendecas legendecas commented Aug 13, 2022

Add cause support to DOMException.

Fixes #969

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


Preview | Diff

@legendecas legendecas changed the title Add DOMException cause Normative: Add DOMException cause Aug 13, 2022
@legendecas legendecas force-pushed the domexception-cause branch 2 times, most recently from de885da to 6c38523 Compare August 30, 2022 06:33
@legendecas legendecas marked this pull request as ready for review August 30, 2022 06:35
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 have some editorial suggestions. This also aligns the IDL with the style we used for addEventListener(). Essentially downplaying the role of the legacy argument type which also simplifies the resulting algorithm a bit.

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
@legendecas
Copy link
Author

@annevk Thank you for the suggestions! Updated.

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

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This looks good with a suggested nit.

However I had another doubt about the general shape here. Which is, should we really be installing an own data property? That's what Error does, but we've already decided to depart from Error for name and message, making those getters instead.

I lean slightly toward consistency with DOMException's existing properties over consistency with Error, given how in the past I tried to encourage consistency with Error and failed (see #378).

But this brings us to my next point, which is, it seems like it's time to ask for implementer support for this change. So let's try pinging: @yuki3, @EdgarChen, @petervanderbeken, and @shvaikalesh and see what they say, both on the general proposal and on whether we should do a data property or a readonly attribute (getter) for cause.

index.bs Outdated Show resolved Hide resolved
@petervanderbeken
Copy link

This looks fine to me, but I'd also like it to be consistent with the other DOMException properties (with a getter). I also think the serialization should include this value.

@yuki3
Copy link

yuki3 commented Sep 6, 2022

+1 to make it an accessor property to be consistent. It's great to have an IDL attribute just like 'name', 'message', and 'code'.

@jasnell
Copy link

jasnell commented Sep 6, 2022

As far as implementor support is concerned, I will implement support in Cloudflare Workers also.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Looking good with small issues.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@legendecas
Copy link
Author

As people seem to be more comfortable with being consistent with IDL attributes on the cause, I've updated the text to define the cause as an IDL attribute on DOMException.

Also, I've updated the steps to include the cause in serialization/deserialization.

@ljharb
Copy link
Contributor

ljharb commented Sep 7, 2022

I'm confused - 'cause' in error is very explicitly designed to determine if an error has a cause or not.

Making it an accessor on the prototype breaks this highly intentional design decision in ecma262. It's meant to be an own property that is completely absent by default.

@bathos
Copy link
Contributor

bathos commented Sep 7, 2022

I was in favor of the accessor for consistency with message (and stack in SpiderMonkey) + the unusualness of Web IDL constructs defining own data properties, but @ljharb’s explanation changed my mind. More reliable API contracts for “whatever value was just thrown at me” is worth a lot more. DOMException is already a lil freaky anyway.

Also tho I sincerely hope nobody ends up needing to spot the distinction between "cause" in new Error("", { cause: undefined }) and "cause" in new Error, if they did need to and couldn’t, that’d be worse. Given cause would typically be a value the cause-bearing-error thrower is just passing along from possibly unrelated code, this might be your only signal that something more spooky’s going down.

@domenic
Copy link
Member

domenic commented Sep 8, 2022

Yeah, I think that's a decision we just won't support on the web platform. It seems fine to not support undefined causes as distinct from absent causes. I gave this feedback back when the proposal was in TC39 and it was rejected. But for IDL and the web platform generally we'll use the web-platform-standard idioms of accessors on the prototype and undefined by default.

@ljharb
Copy link
Contributor

ljharb commented Sep 8, 2022

Google reps were perfectly able to prevent advancing the language proposal - it seems pretty unfortunate to just callously choose to ignore the result of a standards process just because you don’t like the result.

@domenic
Copy link
Member

domenic commented Sep 8, 2022

We're not ignoring the result of the standards process. The standards process you're discussing was about Error. We have our own standards process on the web platform for DOMException.

@ljharb
Copy link
Contributor

ljharb commented Sep 8, 2022

… which inherits from Error, and thus creates user confusion when it deviates from it.

@jasnell
Copy link

jasnell commented Sep 8, 2022

We discussed this today on the WinterCG call. Most of the attendees did not have strong opinions one way or the other with regard to cause as an accessor or cause as a data property. However, those that did have strong opinions were in favor of matching the behavior specified by TC-39 for Error objects.

Specifically from the Cloudflare Workers point of view, because we are aligned with WebIDL, it is easier for us to implement cause using an accessor, and in our typical use case we would treat a missing cause and a cause that is always undefined as being semantically equivalent, however there is a valid argument to make that a cause that is explicitly undefined vs. a cause that just was never provided are very different things and should be treated as such. Unless there's a strong technical argument to make in favor of the accessor option beyond "that's what we do for name and message so let's keep it consistent", I'd prefer that cause on DOMException was defined as an own data property.

@ljharb
Copy link
Contributor

ljharb commented Sep 9, 2022

@saschanaz nope, i think cause is new enough that there are no established patterns - but the only other alternatives are Object.hasOwn(err, 'cause') - which wouldn't work with this accessor approach either - or the incomplete approach of typeof err.cause !== 'undefined', which fails to distinguish between a causeless error and an undefined cause error.

@saschanaz
Copy link
Member

Thanks for the quick answer! That was my impression too.

which fails to distinguish between a causeless error and an undefined cause error.

Sounds like it, but does it / should it practically matter? I think Web IDL generally doesn't try hard to distinguish between explicit undefined and nonexistence (e.g. dictionaries treat explicit undefined as nonexistent), and I believe nobody complained with it.

@bathos
Copy link
Contributor

bathos commented Sep 10, 2022

I think Web IDL generally doesn't try hard to distinguish between explicit undefined and nonexistence (e.g. dictionaries treat explicit undefined as nonexistent), and I believe nobody complained with it.

Whether property absence & property access evaluating to undefined are semantically equivalent or distinct is a characteristic of an API contract, not of properties/objects inherently, and I’d figure the decision would typically be determined by the value space that needs to be expressed. In the Error cause contract (and a few other places in ECMA-262), property absence is used to express a +1 language value space: “all ES values or none”. It’s the least painful mechanism the language has for expressing this empty notion: a “third null” (cursed lol — not-a-value NaV?) couldn’t do the same because when reified as a (public) ES value, it’s “an ES value” and you’re back at square one. The next best thing is pretty gross: a second property (causeIsEmpty: true) for the extra information bit.

Given Web IDL dictionaries don’t need to express this meta value space, it makes sense that they make both cases equivalent and that no one would complain about it; it’s not preventing them from expressing any Web IDL values/not-a-values. I think it seems not far off from "0" / 0 / -0 being semantically equivalent when the dictionary member type is unsigned long: smaller value space = more conversion & conflation desirable? Or at least more is possible, I should say.

The contract in this case though is that of a non-Web IDL API from a user POV, the API of the (hypothetical, but seemingly intended) “super class” (and its other subclasses, notably?). I’m not sure the question of whether the semantic distinction should have been made ought to determine what’s done here. Perhaps the empty/undefined distinction might never help anybody debug anything, but I’m pretty sure cause-always-present will create bugs itself. People will end up using this check regardless and writing tests for whether some catch logic behaves right for each native/platform Error subclass would be unusual.

While I don’t have a strong opinion on whether the distinction was worth making, I haven’t understood why it’d be so off-the-table to — in intuitive, not literal terms — “just pass it to super()”. Is it a pain for implementors?


Aside re: “Web IDL generally doesn't try hard to distinguish between explicit undefined and nonexistence,” I’ve often wished it would try even less hard :)

Chromium console showing history.pushState(1) throwing due to arity — the undefined/absence distinction — while history.pushState(1, undefined) succeeds

@annevk
Copy link
Member

annevk commented Sep 10, 2022

I’ve often wished it would try even less hard :)

Somewhat off-topic, but please file an issue on that against whatwg/html? Seems worth fixing one way or another.

@bathos
Copy link
Contributor

bathos commented Sep 19, 2022

Possibly notable:

console.log("cause" in new WebAssembly.CompileError("xxx", {}));
console.log("cause" in new WebAssembly.CompileError("xxx", { cause: 1 }));

already prints false and true in both Chromium & FF.

@annevk
Copy link
Member

annevk commented Sep 19, 2022

https://webassembly.github.io/spec/js-api/#error-objects

Note: It is not currently possible to define this behavior using Web IDL.

(WebAssembly being partially defined in Web IDL, but not actually being implemented in terms of Web IDL apparently has a couple of other twists to it.)

@jasnell
Copy link

jasnell commented Sep 22, 2022

@bathos

“just pass it to super()”. Is it a pain for implementors?

Conceptually, no, unfortunately the reality is a bit different. For those of us who implement DOMException in C++ using v8's APIs -- which do not provide any mechanism at the C++ level to invoke the Error constructor easily -- we can't simply "pass it to super()"... but that's an implementation problem.

The key issue here for me is that DOMException taking the getter/setter route when all other standardized Error types use the own property route means that users will be forced to either special case DOMException in error handling logic or otherwise be forced to always handle things the way DOMException does, eroding the language level guarantees.

For example,

try {
  doSomething();
} catch (err) {
  // It's now completely ambiguous what 'cause' in err means.

  // I either have to...

  if (err instanceof DOMException) {
    // 'cause' in err is meaningless.
  } else {
    // 'cause' in err has semantic value
  }

  // or always assume 'cause' in error is meaningless.
}

This difference does not just apply to non-browser runtimes. Browser developers will face this same issue. As the other runtimes also implement this, the fact that DOMException does it differently means that developers in general will not be able to rely on it.

Now, that said it's absolutely true that JavaScript allows you to throw literally anything so it can be argued that the mechanism is already unreliable, but since DOMException extends from Error, it really ought to follow the language specified contract for Error or it otherwise should not claim to be an Error.

const ex = new DOMException();
ex instanceof Error;  // true

@jasnell
Copy link

jasnell commented Sep 22, 2022

Alternatively, if DOMException is not going to follow the same semantics with cause as the language spec and other Errors, then DOMException should use a different name for the property so that it does not intentionally introduce a conflict:
domException.whatwgCause for instance.

@domenic
Copy link
Member

domenic commented Sep 22, 2022

The boat has already sailed with message and name.

@jasnell
Copy link

jasnell commented Sep 22, 2022

And it's too late for DOMException to not claim to be an Error, so the option remaining here is that if DOMException is not going to expose cause in a way consistent with Error, it should use a different property name, anything other than cause so that it does not intentionally introduce an inconsistency. Users will still have to special case DOMException, unfortunately, but there's no reason to introduce a new confict.

Additional thought: Having DOMException use a name other than cause for its getter will allow implementations to support both models if they choose, giving them an opportunity to make things easier for developers.

@annevk
Copy link
Member

annevk commented Sep 23, 2022

DOMException is not alone. There are other exceptions that build on it, e.g., RTCError. Which I suppose raises the question as to what should be done about their constructors.

And I think saying that making cause a getter introduces an inconsistency is too broad, as it would also introduce an internal inconsistency for it to not do so.

Also, if we manage to get rid of optional (#905) the natural thing here would be that cause is at least undefined. I'm not even sure what the current binding layer makes of this with dictionaries. It might well already be undefined when missing (or more likely missing when undefined).

@bathos

This comment was marked as off-topic.

aduh95 added a commit to aduh95/uppy that referenced this pull request Nov 7, 2022
aduh95 added a commit to transloadit/uppy that referenced this pull request Nov 7, 2022
@littledan
Copy link
Collaborator

This PR looks good to me as is. I don’t see it as important for JS developers to be able to distinguish between undefined and missing cause—how frequently do you want to check whether there is a cause at all?

I think it would be good if this PR could land so that web error classes have the significant benefit of this cause feature. This utility is much greater than the edge case being discussed above.

For some context for people coming from TC39: it is extremely useful for web specs to stick to WebIDL conventions. Previous attempts to deviate from this pattern (eg streams) were found to be highly complex without developer benefit and were later reverted.

I just don’t see why JS developers should care about this case, so I don’t see why it makes sense to invoke priority of constituencies. This is neutral for JS developers, and helpful for implementers. (It is also helpful for spec writers, but they are lower down on the totem pole.)

Copy link
Member

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

This looks sensible enough; perhaps https://webidl.spec.whatwg.org/#es-creating-throwing-exceptions should be amended as well.

Using an own property seems like it would make specification and implementation more complicated and bugs more likely for little gain.

What is holding this up at this point?

@annevk
Copy link
Member

annevk commented Jul 4, 2023

OP needs to be filled out and @domenic needs to re-review as he explicitly requested changes.

Let me hereby declare that you can consider WebKit as a supportive implementer. You'll need to get an okay from Chromium or Gecko as well.

@ljharb
Copy link
Contributor

ljharb commented Jul 4, 2023

The gain is spec alignment and matching the design philosophy of the feature, which reduces the likelihood of bugs. If web IDL is getting in the way of that, perhaps that should be addressed?

@saschanaz
Copy link
Member

saschanaz commented Jul 4, 2023

The gain is spec alignment and matching the design philosophy of the feature, which reduces the likelihood of bugs. If web IDL is getting in the way of that, perhaps that should be addressed?

And the loss is violating the principle that is applied to all other web platform attributes (as mentioned in #1179 (comment)), which is used for feature detection for example. I think there's no golden answer here, and I'd rather follow the web platform principle as this is a web platform interface.

@petervanderbeken
Copy link

Let me hereby declare that you can consider WebKit as a supportive implementer. You'll need to get an okay from Chromium or Gecko as well.

Gecko will support this (#1179 (comment)).

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nit

index.bs Outdated
@@ -14679,6 +14694,7 @@ Their [=serialization steps=], given <var>value</var> and <var>serialized</var>,
<ol>
<li>Set <var>serialized</var>.\[[Name]] to <var>value</var>'s [=DOMException/name=].</li>
<li>Set <var>serialized</var>.\[[Message]] to <var>value</var>'s [=DOMException/message=].</li>
<li>Set <var>serialized</var>.\[[Cause]] to the [=sub-serialization=] of the value of <var>value</var>'s [=DOMException/cause=].</li>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>Set <var>serialized</var>.\[[Cause]] to the [=sub-serialization=] of the value of <var>value</var>'s [=DOMException/cause=].</li>
<li>Set <var>serialized</var>.\[[Cause]] to the [=sub-serialization=] of <var>value</var>'s [=DOMException/cause=].</li>

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, fixed!

index.bs Outdated Show resolved Hide resolved
@yuki3
Copy link

yuki3 commented Jul 5, 2023

I'm happy to implement cause attribute in Chromium.

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.

Constructing DOMException with causes