subsink: Doesn't this already exist?

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.

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.

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

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());
}

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.

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

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

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