-
Notifications
You must be signed in to change notification settings - Fork 296
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
Requirement for AbortSignal argument to be named 'signal' can be confusing for some APIs. #1112
Comments
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? |
I'm not sure what FLEDGE is, but if it's intended for the web platform I would tend to agree with @domenic. |
Closing this as we're still happy with what we require here. |
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". |
@domfarolino Could you expand on your 👎 vote? Do you see a benefit in restricting the property name to the generic word? |
Uniformity across the platform in AbortSignal integration & naming I think is a good enough reason. |
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
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. 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. const { initialBid, signal } = watchSeller(name);
const bot = new FledgeApiClient({ signal }); |
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.
The text was updated successfully, but these errors were encountered: