Skip to content
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

Update with DOM's abort reason #48

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Conversation

nidhijaju
Copy link
Contributor

@nidhijaju nidhijaju commented Mar 15, 2022

whatwg/dom#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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

@reillyeon reillyeon merged commit 6035b7d into WICG:main Mar 24, 2022
aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 24, 2022
This change updates the start() method and abort steps attached to the
provided AbortSignal so that the reason provided to the AbortController
is used to reject the Promise returned by start.

The web-visible effect of this change is minimal because by default the
abort reason is set to the same AbortError DOMException which was
already being used.

This corresponding specification change is:
WICG/idle-detection#48

Bug: 1309731
Change-Id: I949aeb9d8f23d73a890af8be5da027c9588e812b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3546855
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984953}
@nidhijaju nidhijaju deleted the update-abortreason branch March 25, 2022 03:39
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This change updates the start() method and abort steps attached to the
provided AbortSignal so that the reason provided to the AbortController
is used to reject the Promise returned by start.

The web-visible effect of this change is minimal because by default the
abort reason is set to the same AbortError DOMException which was
already being used.

This corresponding specification change is:
WICG/idle-detection#48

Bug: 1309731
Change-Id: I949aeb9d8f23d73a890af8be5da027c9588e812b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3546855
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984953}
NOKEYCHECK=True
GitOrigin-RevId: bf31b4283028e2c117a4a559feeceea61d5aeb61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants