Closed plinss closed 2 years ago
@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.
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
)?
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 https://github.com/whatwg/dom/issues/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:
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.AbortSignal
need to use the signal's abort reason, instead of creating a new "AbortError"abortReason
stuffThis 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.
Should the
abort
event fired by anAbortSignal
also carry anabortReason
property? That is: instead of "fire an event namedabort
" in step 5 of signal abort, we introduce anAbortEvent
that has anabortReason
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 likefetch()
to reject withx
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.
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?
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.
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.)
@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.
@nidhijaju is working on this issue. Nidhi, can you remove the property?
Yes, I will remove the property.
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.
@atanassov and I looked took a look at this via https://github.com/w3ctag/design-reviews/issues/672. We're wondering if the
abortReason
would be better served as an extension toAbortController
andAbortSignal
rather than being carried an a separate property.