cartant / rxjs-tslint-rules

TSLint rules for RxJS
https://cartant.github.io/rxjs-tslint-rules/
MIT License
371 stars 22 forks source link

Check whether RxJs subscriptions in Angular component are complete or unsubscribed #91

Closed AKlaus closed 5 years ago

AKlaus commented 5 years ago

Is there a way to validate that all RxJs subscriptions are either complete or unsubscribed by the end of the life cycle of Angular component (when ngOnDestroy is called)?

My understanding is that rxjs-tslint-rules doesn't support it at the moment. However, I'd like to see your take on

cartant commented 5 years ago

No, that sounds to me like something that is beyond the capabilities of static analysis. It is, however, something that would be possible with the RxJS DevTools that I'm working on ATM.

AKlaus commented 5 years ago

Hi @cartant, Thank you for a quick response! Do you mean Google Chrome extension RxJa DevTools? Hmm... don't see how it can help...

I've read your blog post, where you were suggesting to use the takeUntil pattern to control subscriptions and added rxjs-no-unsafe-takeuntil rule to rxjs-tslint-rules to enforce it. So I thought, maybe rxjs-tslint-rules can help on a bigger scale.

cartant commented 5 years ago

Nope, it's not that extension.

I'm working on an extension ATM. It's not yet finished and is not publicly available. Essentially, it will be rxjs-spy with many more features and far easier to use.

AKlaus commented 5 years ago

rxjs-spy looks very cool! I'll have a closer look later on (trying to support local Brisbane devs as I can).

cartant commented 5 years ago

BTW, I've made a note of your question, as I think it would be a great feature to add to the DevTools and - hopefully - it will be fairly straightforward.

Antony74 commented 5 years ago

Hi @cartant,

Love what you’re doing with rxjs-tslint-rules.

Regarding the rxjs-prefer-async-pipe rule (which is very relevant to the question about RxJs subscriptions in Angular components), you don’t say why it is preferably to use the async pipe instead of an explicit subscription, am I right in assuming this is to avoid the possibility of subscription leaks?

Certain observables are unproblematic in this respect, because (succeed or fail) they are guaranteed to complete. I can only think of one example of this, but it’s very common:

this.http.get(url).subscribe(

Is there enough type-information or other context available to white-list special cases such as Observable<HttpResponse>? This is a theoretical question, not a feature request at this time, but I am very interested in preventing subscription leaks permissively.

Obviously it’s a completely different ball-game if the stream gets piped and combined with other streams and stuff, and we’d almost certainly have to wait for you RxJS DevTools to be able to untangle that.

cartant commented 5 years ago

I don't use Angular much, these days, but I talk often with developers who do.

rxjs-prefer-async-pipe is not a rule that I've ever used. There are rules in the package that enforce different opinions.

I think it's more than just subscriptions, though. What would you be doing in this.http.get(url).subscribe(...)? I often see people doing this sort of thing, putting a slab of imperative code in the function passed to subscribe and then assigning something to this - to be picked up by change detection.

IMO, that's all rather horrid. And it would be better implemented as a composed observable - using map, etc. to transform the reponse - and that observable could be used with an async pipe. When I did use Angular, I only ever used OnPush change detection and that plays nicely with the async pipe.

So I think it's more than just subscriptions. It's about implementing components in a manner that as declarative a possible. And with OnPush change detection rather than the 'magic' stuff that, IMO, should never have been brought over from AngularJS.

Antony74 commented 5 years ago

Thanks, your comments really helped move forwards our internal discussion here about the subscribe( ) method.