WritableStreamDefaultController.abortReason

[plinss]

@atanassov and I looked took a look at this via w3ctag/design-reviews#672. We're wondering if the abortReason would be better served as an extension to AbortController and AbortSignal rather than being carried an a separate property.

[yutakahirano]

@ricea @domenic @MattiasBuelens what do you think? This sounds like a good suggestion - we can make AbortController.abort() take a reason argument, and add AbortSignal.reason.

[MattiasBuelens]

I like it! 🙂

Should the abort event fired by an AbortSignal also carry an abortReason property? That is: instead of "fire an event named abort" in step 5 of signal abort, we introduce an AbortEvent that has an abortReason and fire that event instead.

Building up on that: when fetch() rejects with an AbortError DOMException as a result of its signal being aborted, should that DOMException have the given abortReason as its cause (as in error.cause)?

[domenic]

I think if this is properly integrated into all calling specifications, such that calling .abort(x) causes methods like fetch() to reject with x instead of with an "AbortError" DOMException, then it would be good.

It would also solve the issue discussed in whatwg/dom#951 about "TimeoutError" DOMException being most appropriate. /cc @annevk. We would make AbortSignal.timeout() call the spec equivalent of controller.abort(new DOMException("...", "TimeoutError")).

The spec changes would probably be something like:

• Easy changes to the base primitives:
  • Each AbortSignal gets an abort reason JavaScript value, initially undefined (or null?).
  • AbortSignal gets a reason getter that returns this's abort reason.
  • AbortController's abort() gets an optional any reason argument. If it is not passed, it creates a new "AbortError" DOMException. It stores the reason in the abort reason for the created signal.
• Changes to all the callers:
  • Fetch:
    • all calls to "Abort fetch" which are triggered by an AbortSignal need to use the signal's abort reason, instead of creating a new "AbortError"
    • maybe also the stream erroring step?
  • Streams:
    • ReadableStreamPipeTo needs to consult the signal's abort reason
    • Remove abortReason stuff
  • Need to also update per https://dontcallmedom.github.io/webidlpedia/names/AbortSignal.html:
    • Web NFC
    • Web Bluetooth
    • App History
    • EyeDropper
    • Idle Detection
    • Prioritized Task Scheduling
    • Web Locks
    • Credential Management
    • Geolocation Sensor

This is a good amount of work, especially updating all the dependent specs/implementations/tests. But I think if we want to do this, we need to go the extra mile and update those too; being in an inconsistent halfway state where .abort(reason) only works with Streams would be bad.

[MattiasBuelens]

Should the abort event fired by an AbortSignal also carry an abortReason property? That is: instead of "fire an event named abort" in step 5 of signal abort, we introduce an AbortEvent that has an abortReason and fire that event instead.

Never mind, event.target.abortReason (or .reason as Domenic named it) would work just as well. 😅

calling .abort(x) causes methods like fetch() to reject with x instead of with an "AbortError" DOMException, then it would be good.

Hmm, I was a bit hesitant about changing the rejection reason of fetch() itself, hence the suggestion for using .cause. But sure, if we want to make this consistent across all standards, then rejecting with the given reason makes the most sense.

[yutakahirano]

Thank you for the summary! Do we need approval from each API spec, or can we make a decision here and then start talking with each API spec independently?

[domenic]

I think we can make the decision here. (Well, really we need to bring the decision to whatwg/dom, but I suspect @annevk will be in favor.) The other specs bought into using a shared primitive with the explicit goal of benefiting from future evolutions to that shared primitive, so they should be OK with such a backward-compatible upgrade.

[annevk]

Yeah, this is good. Solving a couple of problems/requests at once. \o/

("abort reason" being any makes sense and in that case you'd want undefined as the default value. The only slight argument for not doing that is that you could remove the need for the aborted flag, but that seems a lot less important.)

[domenic]

@yutakahirano, can you or your team work on, as a first step, removing abortReason from the WritableStream spec, since it sounds like we're not implementing that? It'd be good to avoid people being misled by the spec as-is.

[yutakahirano]

@nidhijaju is working on this issue. Nidhi, can you remove the property?

[nidhijaju]

Yes, I will remove the property.

[getify]

Apologies for coming in late with this question, but...

Can anyone elaborate on the specific behavior of needing to force reason to be a DOMException if abort() or abort(undefined) are called?

IOW, why shouldn't a developer be able to express the intent that there was no reason for an abort, and then elsewhere be able to detect that intent via an undefined value rather than needing to inspect the name/toString of an exception object? Would allowing that cause some issue that's not obvious?

Moreover, there's a bit of a confusing semantic signal, with the object being an exception... It kind of indicates that reason is required, and that calling abort() without an argument (or with an explicitly passed undefined) is wrong or bad. It never was required before reason was added, and even MDN treats/documents it as optional.

I would just like to understand this specific change. It had some significant negative impacts on my library.