dom icon indicating copy to clipboard operation
dom copied to clipboard

Requirement for AbortSignal argument to be named 'signal' can be confusing for some APIs.

Open morlovich opened this issue 3 years ago • 2 comments

Hi... So I've been adding AbortController/AbortSignal support to the (experimental) FLEDGE APIs, and I think in our case the requirement for the argument field to be called 'signal' in https://dom.spec.whatwg.org/#abortcontroller-api-integration is somewhat undesirable, as the passed in config object to the API already has: auctionSignals sellerSignals perBuyerSignals ... so an unqualified 'signal' in this company feels confusing; I would have much preferred 'abortSignal' if it were spec-permissible.

morlovich avatar Sep 12 '22 14:09 morlovich

It seems better if those "signals" used a different name, e.g. "auctionInfo", so as to avoid conflicting with how the web platform already is using the term "signal" in ~20 APIs?

domenic avatar Sep 12 '22 14:09 domenic

I'm not sure what FLEDGE is, but if it's intended for the web platform I would tend to agree with @domenic.

annevk avatar Sep 12 '22 21:09 annevk

Closing this as we're still happy with what we require here.

annevk avatar May 17 '23 12:05 annevk

I just discovered this and fully agree with the original author that a restriction to use the generic name is very confusing in cases where multiple types of signals could occurr. I'm not sure what the intended benefits are, but this has already caused confusion over here and will likely spread to that spec to cause even more confusion and human attention/time wasted in the future.

Please loosen the requirement to also allow property names ending in "Signal".

mk-pmb avatar Jun 10 '24 18:06 mk-pmb

@domfarolino Could you expand on your 👎 vote? Do you see a benefit in restricting the property name to the generic word? Ideally, the spec should even provide a link to arguments for that restriction. Currently, I can only see the risk/cost. The only argument I found in this thread is that other APIs use the generic name, but that's not a good reason in itself. The other APIs hopefully have good reasons of their own, maybe that in their specific context only one kind of signal makes sense.

mk-pmb avatar Jun 10 '24 18:06 mk-pmb

Uniformity across the platform in AbortSignal integration & naming I think is a good enough reason.

domfarolino avatar Jun 10 '24 19:06 domfarolino

Over in the thread where I originally became confused, I found a solution for the case there, that also may work more generally. I was mislead by some assumptions made in that other thread, and the thread title here, but actually the current spec isn't really evil, just a bit confusing by omission if the reader has a wrong presumption. The requirement

Any web platform API using promises to represent operations that can be aborted must adhere to the following: • Accept AbortSignal objects through a signal dictionary member. — DOM spec, chapter 3.3 "Using AbortController and AbortSignal objects in APIs"

in itself is somewhat useful, because then in scenarios where only one type of signal is plausible, you can write nifty code like this:

function getExampleWebsite() {
  const { abort, signal } = new AbortController();
  const pr = fetch('https://www.example.net/', { signal }).then(resp => resp.text());
  return Object.assign(pr, { abort });
}

And it does in fact not impede on good API design, because we can still use descriptive property names as the default and recommendation in our config objects, we just have to additionally accept AbortSignal on the generic name. It allows to interpret whatever is given in the genericly named property based on its type.

In the original example that started this thread, the API could indeed offer the desired abortSignal option (that will abort with any type of signal) alongside the existing auctionSignals, sellerSignals, and perBuyerSignals. It could take inspiration from the type-aware interpretation so that if signal is set to a SellerSignal, it would be appended to the list of sellerSignals etc. Assuming the existing options have the empty list as default, this could allow for similarly concise code

const { initialBid, signal } = watchSeller(name);
const bot = new FledgeApiClient({ signal });

mk-pmb avatar Jun 11 '24 05:06 mk-pmb