-
Notifications
You must be signed in to change notification settings - Fork 16
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
Pass abortsignal reason to close a subscription #166
Pass abortsignal reason to close a subscription #166
Conversation
2e0a937
to
85e4753
Compare
85e4753
to
c1ac731
Compare
spec.bs
Outdated
@@ -37,6 +37,7 @@ urlPrefix: https://dom.spec.whatwg.org; spec: DOM | |||
for: AbortSignal | |||
text: dependent signals; url: abortsignal-dependent-signals | |||
text: signal abort; url:abortsignal-signal-abort | |||
text: reason; url:abortsignal-abort-reason |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double check, is this needed? I guess it is, but I'm surprised by this. It makes me wonder if we're holding AbortSignal wrong — I would expect this to be exported so other specs that use AbortSignals can propagate reasons similarly.
spec.bs
Outdated
|
||
Issue: Abort with an appropriate abort reason. | ||
Issue: Abort with an appropriate abort reason if none given. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed at all now. We don't need to supply a "fallback" default reason if none was provided.
I cleaned a few things up here. I think it's good to go now so I'll merge. |
SHA: d5a14ab Reason: push, by domfarolino Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #165.
This seems to marry up with Chrome's impl:
Preview | Diff