feat: add rxjs-prefer-angular-takeuntil-before-subscribe rule

macjohnny commented 5 years ago

Description

This rule tries to avoid memory leaks in angular components when calling .subscribe() without properly unsubscribing by enforcing the application of the takeUntil operator before the .subscribe() as well as before certain operators (~~publish, publishBehavior, publishLast, publishReplay,~~shareReplay) and ensuring the component implements the ngOnDestroy method invoking this.destroy$.next() and this.destroy$.complete().

Examples

This should trigger an error:

@Component({
  selector: 'app-my',
  template: '<div>{{k$ | async}}</div>'
})
class MyComponent {
      ~~~~~~~~~~~    component containing subscribe must implement the ngOnDestroy() method

    k$ = a.pipe(shareReplay(1));
                ~~~~~~~~~~~~~~   the shareReplay operator used within a component must be preceded by takeUntil

    someMethod() {
        const e = a.pipe(switchMap(_ => b)).subscribe();
                                            ~~~~~~~~~      subscribe within a component must be preceded by takeUntil
    }
}

while this should be fine:

@Component({
  selector: 'app-my',
  template: '<div>{{k$ | async}}</div>'
})
class MyComponent implements SomeInterface, OnDestroy {
    private destroy$: Subject<void> = new Subject<void>();

    k$ = a.pipe(takeUntil(this.destroy$), shareReplay(1));

    someMethod() {
        const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe();
    }

    ngOnDestroy() {
      this.destroy$.next();
      this.destroy$.complete();
    }
}

This pattern is also used e.g. here https://github.com/angular/components/blob/8da64f4db8d3241024721df8ca3a36b2f47875f5/src/material/select/select.ts#L526-L528

Alternatives

Although a reactive programming approach using the | async pipe without ever calling .subscribe() in the component is more feasible, some code-bases still use the .subscribe() approach.

Caveats

This rule is also triggered by observables that are guaranteed to complete, e.g.

class MyComponent {
  someMethod() {
    this.httpClient.get('/some/path').subscribe((data) => {
      // do something...
    })
  }
}

However, adding takeUntil() in this situation although it would not be needed only adds little overhead, while strictly avoiding memory leaks, so this should be a small trade-off.

TODO

[x] additionally enforce the use of takeUntil before operators like shareReplay(1)
[ ] add fixes where useful
[x] check invokation of the this.destroy$.next() and this.destroy$.complete()

cartant commented 5 years ago

Thanks. This looks good to me. I'll have a closer look at the implementation a little later, but I have a few initial comments:

I'd like to change the rule's name to make it obvious that this is an Angular-specific rule.

This package has another Angular-specific rule - rxjs-prefer-async-pipe - so I'm going to deprecate and rename that one to rxjs-prefer-angular-async-pipe.

Could you rename this PR's rule to rxjs-prefer-angular-takeuntil?
I think it would be better to match the components using a selector that looks for the Component decorator, like this:

ClassDeclaration:has(Decorator[expression.expression.name='Component'])

macjohnny commented 5 years ago

@cartant thanks for your comments

Could you rename this PR's rule to rxjs-prefer-angular-takeuntil?

sure, I will do that tomorrow. How about rxjs-prefer-angular-takeuntil-before-subscribe?

I think it would be better to match the components using a selector that looks for the Component decorator, like this:
ClassDeclaration:has(Decorator[expression.expression.name='Component'])

great, this is what I was looking for, since I copied the idea from

https://github.com/cartant/rxjs-tslint-rules/blob/e339b7a2b587c03cb975b83e7e4e4750cb6fcf1b/source/rules/rxjsPreferAsyncPipeRule.ts#L32-L35

cartant commented 5 years ago

Regarding you Gitter question, I don't think this rule should do anything other than ensure there is a non-nested takeUntil in the operators passed to the pipe call that returns the observable upon which subscribe is called. I don't think it should look at the position of takeUntil. It should, instead, leave that the other rule.

macjohnny commented 5 years ago

Regarding you Gitter question, I don't think this rule should do anything other than ensure there is a non-nested takeUntil in the operators passed to the pipe call that returns the observable upon which subscribe is called. I don't think it should look at the position of takeUntil. It should, instead, leave that the other rule.

@cartant am I right that your statement refers to the rxjs-no-unsafe-takeuntil, not to the newly proposed rxjs-prefer-angular-takeuntil-before-subscribe rule?

macjohnny commented 5 years ago

I think that rxjs-prefer-angular-takeuntil-before-subscribe would make rxjs-no-unsafe-takeuntil obsolete, as it ensures the use of a takeUntil operator right before a subscribe, and in addition before implicitly subscribing operators without teardown-logic:

private operatorsRequiringPrecedingTakeuntil: string[] = [
    "publish",
    "publishBehavior",
    "publishLast",
    "publishReplay",
    "shareReplay"
  ];

Moreover, it ensures the correct argument is used in the takeUntil() operator and that this argument is also called with .next() and .complete() in an ngOnDestroy() method. This strict enforcement of the takeUntil usage should help to avoid 99% of memory leaks in angular components due to missing unsubscribe logic.

cartant commented 5 years ago

It won't make it obsolete because this rule is Angular-only. It is also highly likely that Angular-specific rules will be removed when this package is moved into the RxJS repo. TBH, I think they should probably be elsewhere.

cartant commented 5 years ago

I've read through the code and I have some suggestions - mainly to do with the options - I'll write them up on the weekend. Right now, I'm too tired. Thanks for the work you've put into this. I'm sure some people will find it useful.

ccjmne commented 5 years ago

Hey 👋

Thanks for your contribution! This actually is a rule I tried using in an Angular project... but ended up dropping altogether in favour of thorough reviews :)

In my (little) experience, the caveat you mentioned runs a bit deeper:

The http calls aren't the sole inconvenience: off the top of my head, the Angular SnackBars and Dialogs also use a one-off Observable (for Dialog response, and potential SnackBar action) that does get properly completed whenever the corresponding component gets destroyed.

Additionally, you may already be using something like:

.pipe(
  ...,
  take(1)
).subscribe(...)

... or first(), or maybe other operators I can't recall right now, as well as your own custom operator, like some hypothetical takeUntilIrrelevant().

While the first point is somewhat moot, I think this rule would benefit from allowing the consumer to customise the list of operators that are required to be placed immediately before subscribe.

macjohnny commented 4 years ago

@ccjmne thanks for your remarks

This actually is a rule I tried using in an Angular project... but ended up dropping altogether in favour of thorough reviews :)

I agree, a thorough code review is invaluable, but even there a missing ngOnDestroy might not immediately be caught...

In my (little) experience, the caveat you mentioned runs a bit deeper:

The http calls aren't the sole inconvenience: off the top of my head, the Angular SnackBars and Dialogs also use a one-off Observable (for Dialog response, and potential SnackBar action) that does get properly completed whenever the corresponding component gets destroyed.

Absolutely, the caveat applies to any observable that is guaranteed to complete, but since not effort apart from copy-pasting (I usually have a code-snippet in my IDE for that), I think it is bearable.

Additionally, you may already be using something like:
.pipe(
  ...,
  take(1)
).subscribe(...)
... or first(), or maybe other operators I can't recall right now, as well as your own custom operator, like some hypothetical takeUntilIrrelevant().

even in this situation a takeUntil is necessary. consider this example:

myFormControl = new FormControl();

this.myFormControl.valueChanges.pipe(take(1)).subscribe(...);

If the source observable never emits, the subscription remains. There is even an edge-case (although not a memory leak) with an observable that is guaranteed to complete:

this.httpClient.get('/some-path').subscribe(() => {
  ...
  this.changeDetectorRef.detectChanges();
})

this will result in an error if the request takes long time and the component is destroyed e.g. due to navigation, since the view does not exist anymore.

While the first point is somewhat moot, I think this rule would benefit from allowing the consumer to customise the list of operators that are required to be placed immediately before subscribe.

what would be an example that would need customization?

ccjmne commented 4 years ago

Yes, I thought of the case where, with take(1), if the source Observable never emits, the subscription remains. But when you know you're consuming from a BehaviorSubject, it just feels a bit bad to be instructed to do:

.pipe(
  take(1),
  takeUntil(this.destroyed$)
)

... merely because you couldn't tell your linter that you know what you're doing.

The edge case you mentioned is a good point, though!

About examples that need customisation... I actually wrote a little helper function that takes an OnDestroy and manages its own "destroy$" Subject so I don't have to deal with it.

I export it as takeUntilDestroy and use it like:

.pipe(
  ...,
  takeUntilDestroy(this)
).subscribe(...)

macjohnny commented 4 years ago

@ccjmne

Yes, I thought of the case where, with take(1), if the source Observable never emits, the subscription remains. But when you know you're consuming from a BehaviorSubject, it just feels a bit bad to be instructed to do:
.pipe(
take(1),
takeUntil(this.destroyed$)
)

Omitting takeUntil(this.destroyed$) in this situation can still be dangerous if someone changes the source to e.g. a ReplaySubject and the consumers don't notice that...

About examples that need customisation... I actually wrote a little helper function that takes an OnDestroy and manages its own "destroy$" Subject so I don't have to deal with it.

I agree there are ways to reduce the code to write by hand, but on the other hand it also makes it harder to track whether ngOnDestroy is implemented and correctly calls destroy$.next() and destroy.complete(). While testing this rule with one of our projects, we discovered a component that was missing the ngOnDestroy method.

ccjmne commented 4 years ago

@macjohnny You seem to not have quite understood my last point, which was to have ngOnDestroy automatically decorated (conceptually) so as to always have it properly implemented, and not having to drag along my own destroy$ property in each and every component.

I feel you are very defensive in this whole exchange, while I am actually quite supportive and just meant to say that:

I think this rule would benefit from allowing the consumer to customise the list of operators that are required to be placed immediately before subscribe.

macjohnny commented 4 years ago

@ccjmne I did get the idea of the alternative ngOnDestroy implementation / usage, but I think this would be a custom case that would require a different rule to ensure that there is no memory leak.

I didn't mean to be defensive, I just tried to evaluate whether and why the strict enforcement of takeUntil can help to avoid memory leaks.

nbfr commented 4 years ago

This rule is helpful in most Angular projects to avoid memory leaks.

As long as you stick to the standard pattern using takeUntil(this.destroy$) and calling this.destroy$.next(); this.destroy$.complete(); in ngOnDstroy() this rule should work.

@ccjmne I've seen a project where people wrap the entire observable into an unsubscribe method: this.unsubscribeOnDestroy(this.observable).subscribe(). In this case a custom list of operators will not help. I think this rule should simply cover the standard pattern because it's impossible to know what patterns people might have come up with to handle subscriptions in their projects.

cartant commented 4 years ago

I agree with the suggestion in the comment above: I think the scope of this rule should be as tight as possible. I've been thinking about it and I would also prefer it if it didn't require configuration.

My thoughts:

I'd prefer it if the rule started out be looking for the ngOnDestroy method and if found, inferred the subject name from the method's implementation.
It could then look for subscribe calls and should effect failures for any that don't include a non-nested takeUntil operator that's passed the name that was inferred from the ngOnDestroy implementation.

I would prefer the rule implementation to not ignore subscriptions that include take(1) or first, etc. TBH, those don't really do the same thing as takeUntil as there is no cancellation upon destruction.

macjohnny commented 4 years ago

@cartant thanks for your suggestions, I updated the rule accordingly

I agree with the suggestion in the comment above: I think the scope of this rule should be as tight as possible. I've been thinking about it and I would also prefer it if it didn't require configuration.

I removed the configuration options

My thoughts:

I'd prefer it if the rule started out be looking for the ngOnDestroy method and if found, inferred the subject name from the method's implementation.

It could then look for subscribe calls and should effect failures for any that don't include a non-nested takeUntil operator that's passed the name that was inferred from the ngOnDestroy implementation.

I changed it to the following behavior: when looking for .subscribe() calls and asserting that the last preceding operator is takeUntil, its argument (e.g. this.destroy$) is asserted to be a property access expression, and the property name is added to a list of used destroy subject names. Each name in that list is then checked to be invoked with .next() and .complete() in the ngOnDestroy method.

cartant commented 4 years ago

I would prefer this rule to be kept as simple as possible. I would prefer it to not check the order of the operators within a pipe call. There is another rule that can be used to ensure that takeUntil is used in a safe position. I would prefer this rule to simply check that takeUntil is a non-nested operator in observable chains to which subscribe calls are made.

I think that is reasonable. IMO, it makes little sense to have the logic duplicated across multiple rules.

To me, this rule's responsibilities are to ensure that takeUntil is used wherever an explicit subscribe call is made and that the subjects are notified in ngOnDestroy.

cartant commented 4 years ago

@macjohnny Thanks for the work that you put into this. I merged your work into a separate branch, made some changes and then merged it into master - they are your commits, so you are now a contributor. Thanks again.

The rule is now named rxjs-prefer-angular-takeuntil and it makes no attempt to check the order in which takeUntil operators are used. Instead, it should be used in conjunction with rxjs-no-unsafe-takeuntil.

The implementation itself is now a little more general and handles some additional scenarios.

This package now contains three (mutually-exclusive) Angular-related rules:

this one;
rxjs-prefer-angular-async-pipe which encourages using Component composition to avoid explicit subscribe calls within components; and
rxjs-prefer-angular-composition which enforces Subscription composition for explicit subscribe calls within components.

When/if this package is merged into the official RxJS I might split these Angular-specific rules into a separate package. FWIW, the ESLint versions are going to be in eslint-plugin-rxjs-angular - not in eslint-plugin-rxjs.

cartant commented 4 years ago

4.27.0 has been published and it includes this rule.

cartant commented 4 years ago

https://github.com/cartant/rxjs-tslint-rules#rxjs-no-unsafe-takeuntil

cartant / rxjs-tslint-rules