WICG / idle-detection

A proposal for an idle detection and notification API for the web
Other
68 stars 22 forks source link

Update with DOM's abort reason #48

Closed nidhijaju closed 2 years ago

nidhijaju commented 2 years ago

https://github.com/whatwg/dom/pull/1027 removed the "aborted flag" from DOM's AbortSignal. This change updates the Idle Detection spec with the aborted predicate and rejecting promises with the signal's abort reason, instead of with a new "AbortError" DOMException.


Preview | Diff

reillyeon commented 2 years ago

There is a slight behavior change here. I don't this it is an issue but I'm curious if, considering all the specifications using AbortSignal whether we've seen any issues arising from switching from an AbortError to the actual abort reason. Or maybe this is the only specification which made this mistake.

nidhijaju commented 2 years ago

I'm not aware of any issues that have arose regarding such changes so far where we reject with the abort reason. If you have any suggestions on how this could be better fixed though, I'm also open to that.

nidhijaju commented 2 years ago

I just wanted to add/update my answer to your question. The existing behavior will not be changed unless an abort reason is provided when aborting the signal. So technically speaking, this change would only cause issues if the spec relies on the type of reason that is provided to reject the promise (i.e. relies on the reason for rejection being an "Abort Error" DOMException), as the abort reason could essentially be anything. However, IIUC the Idle Detection API does not depend on that assumption. Is my understanding correct?

reillyeon commented 2 years ago

I just wanted to add/update my answer to your question. The existing behavior will not be changed unless an abort reason is provided when aborting the signal. So technically speaking, this change would only cause issues if the spec relies on the type of reason that is provided to reject the promise (i.e. relies on the reason for rejection being an "Abort Error" DOMException), as the abort reason could essentially be anything. However, IIUC the Idle Detection API does not depend on that assumption. Is my understanding correct?

The current specification language ignores the abort reason and always rejects the promise with an "AbortError" DOMException. It's not a question about the Idle Detection API depending on this behavior but sites depending on it. Currently the code below will print "AbortError" while with this change it will raise a TypeError because error equals undefined since no parameter was passed to abort() and so doesn't have a name attribute.

const controller = new AbortController();
controller.abort();
const idleDetector = new IdleDetector();
try {
  await idleDetector.start({ signal: controller.signal });
} catch (error) {
  console.log(error.name);
}
nidhijaju commented 2 years ago

Currently the code below will print "AbortError" while with this change it will raise a TypeError because error equals undefined since no parameter was passed to abort() and so doesn't have a name attribute.

Actually, if you look at the current DOM spec, controller.abort() will call the "signal abort" algorithm. Step 2 of the algorithm specifies that if an "abort reason" was not given, it is set to a new "AbortError" DOMException, which means that even if no parameter was passed to abort(), error will still be an "AbortError" like before.

reillyeon commented 2 years ago

Currently the code below will print "AbortError" while with this change it will raise a TypeError because error equals undefined since no parameter was passed to abort() and so doesn't have a name attribute.

Actually, if you look at the current DOM spec, controller.abort() will call the "signal abort" algorithm. Step 2 of the algorithm specifies that if an "abort reason" was not given, it is set to a new "AbortError" DOMException, which means that even if no parameter was passed to abort(), error will still be an "AbortError" like before.

Thank you for clarifying the current behavior. When I was last looking into this that behavior had not been specified or implemented in Chromium and so I frequently observed promises being rejected with undefined when abort() was called without a parameter.