search

WICG / observable

Observable API proposal
https://wicg.github.io/observable/
Other
543 stars 12 forks source link

Add the step to check subscriber's active state to next(value)/error(error)/complete() as a shortcut #129

Closed tetsuharuohzeki closed 1 week ago

tetsuharuohzeki commented 3 months ago

Fix https://github.com/WICG/observable/issues/114

Preview | Diff

domfarolino commented 3 months ago

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?

benlesh commented 2 months ago

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. image

domfarolino commented 2 months ago

@benlesh I think your commit touched the wrong method next() instead of error(). Please revert/fix.

benlesh commented 2 months ago

@domfarolino oops, fixed.

tetsuharuohzeki commented 2 months ago

however, it seems that @tetsuharuohzeki you'll need to link your account to a W3C account to pass on of the checks above.

https://github.com/WICG/observable/pull/129#issuecomment-2048472818

@benlesh

I resolved that a moment ago.

Do you think we can make this simplification go a bit further by making wicg.github.io/observable/#subscriber-next-algorithm, wicg.github.io/observable/#subscriber-error-algorithm, and wicg.github.io/observable/#subscriber-complete-algorithm all non-null, and then not clearing them in wicg.github.io/observable/#close-a-subscription?

https://github.com/WICG/observable/pull/129#issuecomment-2033200801

@domfarolino

I would like to have a time to answer it to switch my brain to this spec :) And I think it might be better to talk about it as the new issue.

domfarolino commented 2 months ago

And I think it might be better to talk about it as the new issue.

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!

benlesh commented 3 weeks ago

The additions in this PR look correct to me.

It sounds like we're also discussing some related, but not exactly related, issue of converting a partial observer or next handler to a Subscriber, in which case if no error handler is provided, a reportError is put in its place. I thought we were already doing that?

In any case, IMO, this PR is good to stand on its own. What do you think, @domfarolino ?

domfarolino commented 3 weeks ago

It sounds like we're also discussing some related, but not exactly related, issue of converting a partial observer or next handler to a Subscriber, in which case if no error handler is provided, a reportError is put in its place. I thought we were already doing that?

I don't think so? I'm not sure what a partial observer is, or what you mean about the error handling stuff. The only thing I'm discussing / requesting in https://github.com/WICG/observable/pull/129#issuecomment-2033200801 and https://github.com/WICG/observable/pull/129#discussion_r1560113116 is that we fully migrate away from the old world of making "next algorithm", "error algorithm", and "complete algorithm" signals for inactivity, by no longer making them nullable, and no longer nulling them out in the "Close" algorithm. I think that would just make it more likely that we would remember to not use those as signals for inactivity in future PRs, and instead solely use the "active" boolean as this PR starts to do.

It should be a very simple change, but I want to make sure that @tetsuharuohzeki agrees.

tetsuharuohzeki commented 1 week ago

I'm sorry about no response for this issue.

I'm working today to make Subscriber concept's next/error/complete algorithm non-null as @domfarolino's suggestion. I seem it's possible but I also checking it's valid again.

tetsuharuohzeki commented 1 week ago

@domfarolino

Let me know if that doesn't make sense.

I concern that your suggestion makes to allow infinite recursion accidentally.

The current spec calls error steps or complete steps after "close subscription" (set active=false) and run signal abort to stop a subscription chain completely.

But your suggestion is that calling error steps or complete steps before "close subscription" and "run signal abort". That does not wait to stop a subscription chain.

I fear your suggestion that may introduce a chance probably for accidental infinite recursion. It should be discussed in a new topic.

domfarolino commented 1 week ago

But your suggestion is that calling error steps or complete steps before "close subscription" and "run signal abort".

No my suggestion is still calling the error steps or complete steps after Close. I just think we can get rid of these lines:

  1. https://pr-preview.s3.amazonaws.com/WICG/observable/129/98a4f07...tetsuharuohzeki:f80ab90.html#dom-subscriber-next:~:text=Let%20error%20algorithm%20be%20this%20%27s%20error%20algorithm
  2. https://pr-preview.s3.amazonaws.com/WICG/observable/129/98a4f07...tetsuharuohzeki:f80ab90.html#dom-subscriber-next:~:text=Let%20complete%20algorithm%20be%20this%20%27s%20complete%20algorithm
tetsuharuohzeki commented 1 week ago

But your suggestion is that calling error steps or complete steps before "close subscription" and "run signal abort".

No my suggestion is still calling the error steps or complete steps after Close. I just think we can get rid of these lines:

1. https://pr-preview.s3.amazonaws.com/WICG/observable/129/98a4f07...tetsuharuohzeki:f80ab90.html#dom-subscriber-next:~:text=Let%20error%20algorithm%20be%20this%20%27s%20error%20algorithm

2. https://pr-preview.s3.amazonaws.com/WICG/observable/129/98a4f07...tetsuharuohzeki:f80ab90.html#dom-subscriber-next:~:text=Let%20complete%20algorithm%20be%20this%20%27s%20complete%20algorithm

Ah, that's make sense. Okay. I'll address them.

tetsuharuohzeki commented 1 week ago

I pushed the addressed commit.