-
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
subscribe() should have a symmetric twin unsubscribe() #148
Comments
Unlike |
Exactly. I'll also point to #12 for more discussion on the integration with |
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. |
And both make it impossible to solve the synchronous fire hose probably discussed in #12. Please read that issue fully. |
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. |
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. |
For this, one pattern that appears to be becoming more popular would be having
If the return value also supported the
Returning the
|
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
|
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.
This is a little strange though because you'd you'd have two ways to abort the subscription:
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 |
I'm going to add the "possible future enhancement" label since I think adding the return callback from |
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". 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"? Ignoring the (not yet) returned subscription or not passing an optional AbortSignal is a really easy way to introduce leaks. |
… 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.)The text was updated successfully, but these errors were encountered: