wardbell/subsink

Doesn't this already exist?

Closed this issue · 18 comments

Hi Ward I saw your talk with Sander at ng-conf and really enjoyed it. I saw your example code and it struck me I am already doing this without any 3rd party lib, just stock rxjs. It looks like this:


import { Observable, Subscription } from 'rxjs';

// Component decorator here ...

subscriptions = new Subscription();

ngOnInit() {
  // elipsis symobolizes omitted code
  this.subscriptions.add(this.config$.subscribe(...);
  this.subscriptions.add(this.store.pipe(select(getErrorMessage)).subscribe(...)
  this.subscriptions.add(this.dialogState$.subscribe(...);
}
ngOnDestroy() {
  this.subscriptions.unsubscribe();
}

Maybe I'm missing something but seems like this already comes with rxjs' Subscription object.

Funny, I didn't know you could use a single Subscription like that. I've always done this, which is about as much code as this library requires:

// Imports ...

private _subscriptions: Subscription[] = [];

// ...

ngOnInit() {
	this._subscriptions.push(this._store.pipe(select(someSelector)).subscribe(...));
	// ...
}

// ...

ngOnDestroy() {
	this._subscriptions.forEach(x => x.unsubscribe());
}

Or u can use

  subs: Subscription = new Subscription();
  // ...
  ngOnDestroy() {
    if (this.subs) this.subs.unsubscribe();
  };

Came here to say the same thing.

And you can just abstract away that too in a BaseComponent and have ngOnDestroy() on that, chances are the only thing in the OnDestroy would be the unsubscribe call anyways, but if not, then you can always just call super.ngOnDestroy()

And if he really wanted to show something fancier, then it would've been this: https://www.npmjs.com/package/ngx-auto-unsubscribe

I like the ease of use of subsink. Just an assignment without any braces :-)
this.subs.sink = observable$.subscribe(...); // easy syntax

Is there any real advantage of using SubSink over Subscription from RxJs? I've seen a lot of guys at ng conf praising SubSink.

I like the ease of use of subsink. Just an assignment without any braces :-)
this.subs.sink = observable$.subscribe(...); // easy syntax

@markus-heinisch In the array syntax this would still work, though it's kinda silly

this._subscriptions[0] = observable$.subscribe(...);
this._subscriptions[1] = observable2$.subscribe(...);
// etc...

I'm also trying to better understand the performance/syntactical benefits of SubSink over boring-ass RxJs:

http://reactivex.io/rxjs/class/es6/Subscription.js~Subscription.html

Additionally, subscriptions may be grouped together through the add() method, which will attach a child Subscription to the current Subscription. When a Subscription is unsubscribed, all its children (and its grandchildren) will be unsubscribed as well.

@markus-heinisch stock rxjs does a single subscription too:

sub: Subscription;
this.sub = observable$.subscribe(...); 

And of course an rxjs Subscription object does multiple subscriptions as demonstrated in my first comment with the add() method.

If you use SubSink, it will just pointlessly bloat your bundle. I think @wardbell had the best intentions when he made this, but it shouldn't be used when you get this functionality with code that is already shipping with your application. I don't think @johnpapa or @DanWahlin would have advertised it in their ng-conf talks this year if they knew that.

Let's not be over dramatic with the "pointless bloat" comment. It's tiny, so tiny you could just copy it in. I prefer it because I don't like wrapping parentheses around multi-line subscriptions when calling add(). I've used Subscription (exactly as you mention at the beginning), @AutoUnsubscribe, using subscriptions directly, takeUntil (which has problems in some scenarios), etc. and prefer the simplicity of subsink.sink. Some may argue the semantics of using "sink" but I'm fine with it. Personal preference after trying everything out there - has extremely minimal overhead. If you prefer subscription.add() and wrapping the parentheses around subscriptions then that's definitely a valid option. But, it's not the only option.

Except it kinda is pointless bloat. Using it is about the same amount of code as what it's doing in the background.

The only thing that it does have an advantage for is that you can add multiple things at once, I agree there. But that case isn't super common in my experience. Even if it is, the code

this._subscriptions = this._subscriptions.concat([
    someObservable$.subscribe(...),
    someObservable2$.subscribe(...),
    // etc.
]);

isn't that bad at all in the cases you'd have to use it.

Hi everyone!

Let’s be kind and respectful here and avoid words like “pointless” and others used here. The code is optional. People wrote this. If you don’t want to use it that’s ok. Ward and I have often said you can do this on your own too. We’ve shown this as well.

Let’s remember to create the community we want to be in.

@DanWahlin @johnpapa Thanks for responding. Maybe I got carried away saying "pointless" bloat. I guess it's frustrating when our community leaders (you guys) are advertising something that is unnecessary.

Dan yes, you can keep repeating this.subs.sink = observable1$; this.subs.sink = observable2$; but yes I do think that's bad semanticly. John has frequently advocated for clear, readable code and that is not. The other way of using add() is identical to an rxjs Subscription.

I might add that I very kindly brought this up when I opened this issue, and it has received no response in the 5 months since.

Unnecessary? So what I like to do is irrelevant because you don't like it? You have to pick your poison. I personally think subsink.sink is very clean having tried every other option. Way cleaner than calling add() and having to make sure the parens are matched up correctly. That makes more of a mess IMO.

The biggest mistake any of us can make though is assuming that "our" way is the right way. Never the case. Everyone has their own preferences and we all (and I include myself) need to respect that. If you don't like subsink.sink don't use it....very simple solution.

I think we've beat this horse enough. Use it if you like it. Ignore it if you don't. Cheers to all. Comments closed.

Behavior of the native Subscription and subsink is different. See stackblitz example https://stackblitz.com/edit/rxjs-qmdpa9?file=index.ts

So If i understand it correctly, the SubSink just 'clears' the existing subscriptions, while the native Subscription, clears existing subscriptions and shuts down 'the pipe'. Is this assumption correct ?

@Bogdan0205 Once we call unsubscribe method on the RxJS Subscription, it will be marked as closed and any new subscription we append to it, will be immediately unsubcribed. On the other hand, we can reuse Subsink as it holds the subscriptions in a simple array. On unsubscribe call, it loops through the array and calls unsubscribe on all the subscriptions, array is then emptied.

You usually don't notice the difference, because you call unsubscribe in the ngOnDestroy hook, so after the unsubscription, the whole Subscription object is destroyed.