-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add the step to check subscriber's active state to next(value)/error(error)/complete() as a shortcut #129
base: master
Are you sure you want to change the base?
Add the step to check subscriber's active state to next(value)/error(error)/complete() as a shortcut #129
Conversation
…error)/complete() as a shortcut
1fd6812
to
ad708d0
Compare
Thanks! Do you think we can make this simplification go a bit further by making https://wicg.github.io/observable/#subscriber-next-algorithm, https://wicg.github.io/observable/#subscriber-error-algorithm, and https://wicg.github.io/observable/#subscriber-complete-algorithm all non-null, and then not clearing them in https://wicg.github.io/observable/#close-a-subscription? |
spec.bs
Outdated
@@ -210,6 +212,8 @@ The <dfn attribute for=Subscriber><code>signal</code></dfn> getter steps are to | |||
<div algorithm> | |||
The <dfn for=Subscriber method><code>error(|error|)</code></dfn> method steps are: | |||
|
|||
1. If [=this=]'s [=Subscriber/active=] is false, then return. |
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.
This is correct, but there's an additional step. If the Subscriber/active
is false
, we'll want to report the |error|
with reportError
, because it can't be handled and we don't want it to be completely swallowed or ignored. cc @domfarolino
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.
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'm not quite sure how the build is failing, maybe @domfarolino needs to look at it, or maybe it just needs rebased... however, it seems that @tetsuharuohzeki you'll need to link your account to a W3C account to pass on of the checks above. |
@benlesh I think your commit touched the wrong method |
…s inactive." This reverts commit f1b0503.
@domfarolino oops, fixed. |
I resolved that a moment ago.
I would like to have a time to answer it to switch my brain to this spec :) |
I think you are proposing to do this in a separate issue/follow-up, but I think this should be done in this PR. It is a simple change that is very inline with the changes this PR makes. I view the changes I've requested as a way to "finish up" the work started here, which would be great to do here! |
Fix #114
Preview | Diff