-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: add rxjs-prefer-angular-takeuntil-before-subscribe rule #107
feat: add rxjs-prefer-angular-takeuntil-before-subscribe rule #107
Conversation
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:
|
@cartant thanks for your comments
sure, I will do that tomorrow. How about
great, this is what I was looking for, since I copied the idea from rxjs-tslint-rules/source/rules/rxjsPreferAsyncPipeRule.ts Lines 32 to 35 in e339b7a
|
Regarding you Gitter question, I don't think this rule should do anything other than ensure there is a non-nested |
@cartant am I right that your statement refers to the |
I think that private operatorsRequiringPrecedingTakeuntil: string[] = [
"publish",
"publishBehavior",
"publishLast",
"publishReplay",
"shareReplay"
]; Moreover, it ensures the correct argument is used in the |
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. |
check takeuntil before shareReplay, etc.
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. |
rename to rxjs-prefer-angular-takeuntil-before-subscribe
Hey 👋 Thanks for your contribution! In my (little) experience, the caveat you mentioned runs a bit deeper:
Additionally, you may already be using something like:
... or 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 |
fix NPE for complex class
@ccjmne thanks for your remarks
I agree, a thorough code review is invaluable, but even there a missing
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.
even in this situation a
If the source observable never emits, the subscription remains.
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.
what would be an example that would need customization? |
Yes, I thought of the case where, with
... 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 I export it as
|
Omitting
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 |
@macjohnny You seem to not have quite understood my last point, which was to have I feel you are very defensive in this whole exchange, while I am actually quite supportive and just meant to say that:
|
@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 |
This rule is helpful in most Angular projects to avoid memory leaks. As long as you stick to the standard pattern using @ccjmne I've seen a project where people wrap the entire observable into an unsubscribe method: |
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 would prefer the rule implementation to not ignore subscriptions that include |
remove configuration options
@cartant thanks for your suggestions, I updated the rule accordingly
I removed the configuration options
I changed it to the following behavior: when looking for |
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 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 |
@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 The rule is now named The implementation itself is now a little more general and handles some additional scenarios. This package now contains three (mutually-exclusive) Angular-related rules:
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 |
|
Description
This rule tries to avoid memory leaks in angular components when calling
.subscribe()
without properly unsubscribing by enforcing the application of thetakeUntil
operator before the.subscribe()
as well as before certain operators (publish
,publishBehavior
,publishLast
,publishReplay
,shareReplay
) and ensuring the component implements thengOnDestroy
method invoking
this.destroy$.next()
andthis.destroy$.complete()
.Examples
This should trigger an error:
while this should be fine:
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.Related
#91
Caveats
This rule is also triggered by observables that are guaranteed to complete, e.g.
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
takeUntil
before operators likeshareReplay(1)
this.destroy$.next()
andthis.destroy$.complete()