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

subscribe() should have a symmetric twin unsubscribe() #148

Open
mk-pmb opened this issue Jun 6, 2024 · 11 comments
Open

subscribe() should have a symmetric twin unsubscribe() #148

mk-pmb opened this issue Jun 6, 2024 · 11 comments
Labels
possible future enhancement An enhancement that doesn't block standardization or shipping

Comments

@mk-pmb
Copy link

mk-pmb commented Jun 6, 2024

… so that wrapper functions can use their same existing function reference for both, rather than having to instantiate an AbortController.

Edit: Some people may want to protect their subscription from "unauthorized" unsubscribe(). We can easily achieve this by exempting subscriptions that have been made with an explicit AbortController signal for unsubscribing, or where false was given instead of such a signal. (The latter may be moot if there's a reliable built-in "never" signal available at no additional memory cost.)

@Jamesernator
Copy link
Contributor

… so that wrapper functions can use their same existing function reference for both, rather than having to instantiate an AbortController.

Unlike .addEventListener, subscribing to an observable with the same function/observer multiple times produces multiple subscriptions. Originally Observable returned a Subscription object with an .unsubscribe method, however this was changed so that it behaves correctly in both sync and async cases.

@domfarolino
Copy link
Collaborator

Exactly. I'll also point to #12 for more discussion on the integration with AbortController/AbortSignal.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 7, 2024

I see. I'm not 100% sure I understood #12 but it sounds like I could get a Subscription object with an .unsubscribe() method, or an opaque value (like from setTimeout) that I can pass into the Observable's .unsubscribe(). Both would be ok for me, even if not as symmetric as I had hoped. If one of them has an obvious performance and memory benefit, I'd prefer that.

@domfarolino
Copy link
Collaborator

And both make it impossible to solve the synchronous fire hose probably discussed in #12. Please read that issue fully.

@mk-pmb
Copy link
Author

mk-pmb commented Jun 8, 2024

I did read it fully. It's just that the example source code there is kinda cryptic to me, partially because it uses generic variable names like "subscriber" when there seem to be at least three different kinds of subscribers involved, so any time I see it I have to try and deduce from context which one it's refering to. Doesn't help either that I have to pay careful attention to whether the variable name carries a final "s" to mark the plural. "Inner" and "outer" aren't very obvious either.

I hope someone will find time to make a better example program to demonstrate the firehose problem. (Edit: I'll ask in the other thread if we can open-source the example and maybe even make it a test case, then I can play around with it.)

As it currently stands, it seems like I could trigger my AbortController at any time if I instantiate one and keep a reference to it around, so I really struggle to see why it would be hard to delegate the reference keeping to the Observable and delegate the controller triggering to a convenience method.

@domfarolino
Copy link
Collaborator

I think if you played around with the synchronous firehose example a bit more to understand it, it would be clear why a convenience method would make it impossible to react to the synchronous firehose case.

@jasnell
Copy link

jasnell commented Jun 10, 2024

For this, one pattern that appears to be becoming more popular would be having subscribe(...) return a disposable token that can be used to subscribe. So, something like...

const unsubscribe = observable.subscribe(...);

// The return value is a function that, when called, unsubscribes and cancels the subscription
unsubscribe();

If the return value also supported the Symbol.dispose (from the TC39 Explicit Resource Management proposal) then we could also just do...

using unsubscribe = observable.subscribe(...);

Returning the unsubscribe as a function makes it fairly trivial to incorporate canceling the subscription in response to other events, for example:

someSignal.onabort = observable.subscribe(...);

@mk-pmb
Copy link
Author

mk-pmb commented Jun 11, 2024

someSignal.onabort = observable.subscribe(...);

Indeed, that would be a really neat syntax. If we use subscription objects instead (in order to maybe have other future functionality on them), we could make it bound by default (or be independent of "this") so you could still do it as

someSignal.onabort = observable.subscribe(...).unsubscribe;

@domfarolino
Copy link
Collaborator

domfarolino commented Jun 11, 2024

For this, one pattern that appears to be becoming more popular would be having subscribe(...) return a disposable token that can be used to subscribe.

Hmm, interesting. Where is this becoming more popular? Is this pattern making its way on the web platform? I'm not sure I've seen it a whole lot.

Returning the unsubscribe as a function makes it fairly trivial to incorporate canceling the subscription in response to other events, for example:

someSignal.onabort = observable.subscribe(...);

This is a little strange though because you'd you'd have two ways to abort the subscription:

  1. The signal passed into subscribe() (if you're concerned about the Observable synchronously emitting events)
  2. The magical callback function returned from subscribe()

I guess the upside is that if you don't want to / need to cater to the synchronous firehose scenario, you can use the (maybe?) more ergonomic unsubscribe callback returned from subscribe()? Maybe I'm not opposed to adding this capability, but it might be a little confusing.

@domfarolino domfarolino added the possible future enhancement An enhancement that doesn't block standardization or shipping label Jun 11, 2024
@domfarolino
Copy link
Collaborator

I'm going to add the "possible future enhancement" label since I think adding the return callback from subscribe() could be an enhancement that need not block the initial API design.

@flensrocker
Copy link
Contributor

The more I think about the "symmetry" thing on subscribe/unsubscribe and the synchronous firehose problem - I don't think I would want a "subscription" to be returned from "subscribe" anymore.

And having two ways of doing the same thing (stop the observer), we have to explain to new developers the difference and when/why they have to use which. And I would almost always explain, how to use "takeUntil", because that's the most declarative one.

With the AbortController we already have the other side of "subscribe", but it's called "abort".
We cannot change "abort", but we can name "subscribe" whatever we want to make this the "opposite" of "abort" (I'm not a native english speaker so I don't know if there's a word for this).

Just as a thought experiment: What, if we don't really use the words "subscription" (at the moment there is no subscription returned) and "subscribe"/"subscriber" and use "observe"/"observer"? Will this match semantically better with "abort"?
Or to reflect the "takeUntil" something like "observeUntil" where we are more or less forced to specifiy the "until"?

Ignoring the (not yet) returned subscription or not passing an optional AbortSignal is a really easy way to introduce leaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
possible future enhancement An enhancement that doesn't block standardization or shipping
Projects
None yet
Development

No branches or pull requests

5 participants