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

Compose subscriptions in single operation #1583

Closed
matthewwithanm opened this issue Apr 6, 2016 · 10 comments
Closed

Compose subscriptions in single operation #1583

matthewwithanm opened this issue Apr 6, 2016 · 10 comments

Comments

@matthewwithanm
Copy link

Small feature request:

I'd like a way to compose multiple subscriptions in a single operation. Something like Subscription.create(...subscriptions: Array<Subscription>): Subscription. (I'm assuming that the constructor doesn't do this for perf reasons, so supporting var-args in add would be my second choice.)

If this is something that would be merged, I can put together a PR.

@kwonoj
Copy link
Member

kwonoj commented Apr 6, 2016

I'm not sure if I understood correctly (please point me if I'm wrong), is this means flowwise similar as below? (pseudo, course)

const existingSubs:Array<Subscription> = [....];
const sub = new Subscription();

existingSubs.forEach(x => sub.add(x));

@matthewwithanm
Copy link
Author

Yup! Just a way to do that with a single expression/function call.

@kwonoj
Copy link
Member

kwonoj commented Apr 6, 2016

I'd consider this could be achieved relatively short code externally, without introducing additional interface to subscription. Would you able to elaborate use case bit more?

@matthewwithanm
Copy link
Author

Yes, this is definitely a convenience. For use cases, you can check out how we use Atom/event-kit's CompositeDisposable in Nuclide. Basically, we create a composite subscription at the same time as we subscribe and then treat the result as essentially immutable:

new CompositeDisposable(
  a.subscribe(...),
  b.subscribe(...),
  c.subscribe(...),
);

Often, this is used as an argument to another function, e.g.

doSomething(
  new CompositeDisposable(
    ...
  ),
);

It's certainly possible to do this by iterating over a list of subscriptions and adding them, but not nearly as elegant.

At first I thought that I could achieve something similar by chaining .add() calls but now I see that that interface isn't actually fluent (i.e. if I do a.add(b).add(c) and unsub b, it'll unsubscribe c too).

@kwonoj
Copy link
Member

kwonoj commented Apr 9, 2016

I will not conclude this by myself and would like to wait other suggestions as well - as implementation itself is somewhat trivial only concern I'm having is api surface to Subscription where I'd prefer minimum surface as much.

@benlesh
Copy link
Member

benlesh commented Apr 9, 2016

Current possible solutions

Technically, right now you could do subscription.add(sub1).add(sub2).add(sub3). But that's less than ideal, I suppose. Because you'd be making sub2 a descendant of sub1.

Also, right now you could do this: new Subscription(() => [sub1, sub2, sub3].forEach(x => x.unsubscribe()))

Proposal

We could have new Subscription optionally accept functions or subscriptions in rest args.

new Subscription(someUnsubFunc, subscription1, subscription2);

The risk here is that Subscription becomes too polymorphic and it negatively impacts performance. Another risk is that the implementation inefficiently allocates an array or other resources before it has to.

@matthewwithanm
Copy link
Author

But that's less than ideal, I suppose. Because you'd be making sub2 a descendant of sub1.

Yeah, this was actually the source of a pretty nasty bug of mine! I had assumed that a.sub(b) was equivalent to a.sub(() => { b.unsubscribe(); }). Oops!

The constructor option would definitely be the least surprising API IMO but like I mentioned in the OP, add would be alright. You could avoid the unnecessary array allocation (without accessing arguments which I guess has its own optimization issues) by adding an isArray check to add. new Subscription().add([a.subscribe(), b.subscribe(), c.subscribe()]) is definitely not as appealing though and I guess you'd be throwing away the original subscription too, which also makes this a surprising API. Of course a new static is always an option, e.g. mirroring ES6 Arrays with Subscription.of().

ghost pushed a commit to facebookarchive/nuclide that referenced this issue Apr 13, 2016
Summary:This change upgrades us to the RxJS 5 beta. It includes:

* Update Flow definitions to reflect new APIs
* Add `DisposableSubscription` and `CompositeSubscription` for dealing with new Subscription API (See also ReactiveX/rxjs#1583)
* Add `bufferUntil` to replace some `buffer()` usages (See ReactiveX/rxjs#1610)
* Remove rx-dom
* Update package.json
* Update all code to use new APIs

Reviewed By: peterhal

Differential Revision: D3131543

fb-gh-sync-id: 437c7b90d6e1fd8e463a57eb341146bbc4e9bbd3
fbshipit-source-id: 437c7b90d6e1fd8e463a57eb341146bbc4e9bbd3
@BrainCrumbz
Copy link
Contributor

Before going to ask in the wild, I thought to quickly try here. Sorry if this is a little bit off-topic.

Does anyone know how to extend Subscription TypeScript class (extend as in adding a method to JS prototype, or as in defining a C# extension method) with a new method? We'd like to write code like this:

let rootSubscription = new Subscription();

someObservable$
  .subscribe(someValue => doSomething(someValue))
  .addTo(rootSubscription);

anotherObservable$
  .subscribe(anotherValue => doSomethingElse(anotherValue))
  .addTo(rootSubscription);

addTo() extended method would be something like this:

function addTo(
    collectorSub: Subscription, // this is the explicit parameter
    thisSub: Subscription  // this is the "implicit" instance
  ): void {
  collectorSub.add(thisSub);
}

Thanks, and sorry again for temporarily diverting this issue.

@benlesh
Copy link
Member

benlesh commented Feb 21, 2017

closing this as inactive. Feel free to reopen if needed.

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants