-
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
Proposal: DOMException.ignoreAborts(promise) #933
Comments
From a point of view of Blink-V8 bindings, IIUC, you're going to implement a new (static?) IDL operation (which takes a promise and returns a promise) and no new mechanism seems necessary. Then, no objection of course. (The prototype chain of DOMException is a bit special, but I don't think it matters in this case.) I'm sorry that I'm not familiar with this sort of code and honestly I don't understand its usefulness. Probably it's very common, I guess? Only my question is that AbortError is so special? Do we want to ignore other kinds of errors? Like |
Yep! I just wanted to check, since you all are really the best "owners" for
Yeah, Some of the original designs for promise cancelation (what is now |
Shouldn't |
@annevk I think a reasonable argument could be made for any of |
I think this is a great idea, though I realized after looking at some places in our codebase where we’re doing “isAbortError” checks that it seems like it might be a more niche thing than I initially supposed (or maybe we’re just weird) and that it may not be helpful in our case. It seemed like the most common places where we have these checks is in async functions where control flow should move to the catch block if an abort occurs. If I’m not mistaken, if we tried to leverage this API, we’d end up having to move the error handling that occurs within these catch blocks elsewhere in each such case — and it might end up being a net loss for removing boilerplate / improving clarity. async butterBiscuits({ signal }) {
try {
this.biscuits.butter = await fetch('https://butternet/the-good-stuff?lots', { signal });
} catch (err) {
if (!isAbortError(err)) {
alerts.push('a TERRIBLE thing happened');
throw err;
}
}
} Note that if the fetch here aborts, we wouldn’t want Is there a pattern I’m missing where |
@bathos, your example would be rewritten as follows: async butterBiscuits({ signal }) {
try {
const result = await DOMException.ignoreAborts(fetch('https://butternet/the-good-stuff?lots', { signal }));
if (result) {
this.biscuits.butter = result;
}
} catch (err) {
alerts.push('a TERRIBLE thing happened');
throw err;
}
} |
Right — so the if-statement “artifact” gets moved up, but hasn’t actually gone away. And although that example doesn’t illustrate it cause it was 1:1, it multiplies it, too — an additional new if statement for each abortable-awaited-thing, instead of just one for all of them. |
I personally think it would be most natural as a method of await Promise.ignoreAbort(somePromise);
// with the proposal
await.ignoreAbort somePromise; And as an added benefit it would be a less-awkward thing to perhaps standardize later in the TC39 than "DOMException" which is really weird legacy name that confers little meaning (especially in this context). |
This doesn't make much sense on Promise since it's specifically about ignoring "AbortError" DOMExceptions. |
Here're my thoughts. Can this be a userland function instead? I can imagine people asking for other related functions, like say |
@domenic @MattiasBuelens how has the thinking evolved here now that |
You're right, it's no longer sufficient to only ignore AbortSignal.prototype.ignoreAborts = function (promise) {
promise = Promise.resolve(promise);
return promise.catch(e => {
if (this.aborted && e === this.reason) {
// swallow exception
} else {
// rethrow
throw e;
}
});
}; which authors could use as: async function butterBiscuits({ signal }) {
try {
const result = await signal.ignoreAborts(fetch('https://butternet/the-good-stuff?lots', { signal }));
if (result) {
this.biscuits.butter = result;
}
} catch (err) {
alerts.push('a TERRIBLE thing happened');
throw err;
}
} It's kind of weird that you have to use |
I think in most cases you would only bother with it at the same level you create the |
@MattiasBuelens thanks, though it seems such an approach would still place special value on aborting. Also, Perhaps @yuki3's #933 (comment) is more applicable now, especially if we also introduce |
I still think In particular, given the motivating case for branching out, i.e. |
That's consistent with our experience. Timeout is a result. We do actually have checks like this in other cases, including one spot where there are three DEX types that are expected and not considered exceptional: let shouldIgnore =
check.isAbortError(err) ||
check.isNotAllowedError(err) ||
check.isNotSupportedError(err); But this example only makes sense in its very local context. The way AbortError models a type of completion instead of an abrupt result seems special / a general semantic related to lower level concerns than the others. It's like |
Hmm, Note that this was quite the fuss in the dotnet world a bit back dotnet/runtime#21965 |
Ported from whatwg/dom#881.
Consider the following code we're adding as an example to the Streams Standard in whatwg/streams#1059:
Wouldn't it be nice if we could do this instead?
The semantics here are roughly
This is small enough that I'd be willing to implement it in Chromium, if given the binding team's blessing. (@yuki3, thoughts?)
Any thoughts from other implementers?
The text was updated successfully, but these errors were encountered: