Skip to content
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

ngOnDestroy is not implemented raised by prefer-takeuntil rule when ngOnDestroy is in a super class #1

Open
ldulary opened this issue Jun 24, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@ldulary
Copy link

ldulary commented Jun 24, 2020

Considering this super class :

import { Directive, OnDestroy } from '@angular/core';
import { Subject } from 'rxjs';

@Directive()
export class Destroy implements OnDestroy {
    protected destroyed$ = new Subject();

    public ngOnDestroy() {
        this.destroyed$.next();
        this.destroyed$.unsubscribe();
    }
}

In the components extending Destroy, having takeUntil(this.destroyed$) before each subscribe should be enough to fullfill prefer-takeuntil rule.
But there is the following issue :
error `ngOnDestroy` is not implemented rxjs-angular/prefer-takeuntil

@ldulary ldulary changed the title ngOnDestroy is not implemented when it is in a super class in prefer-takeuntil rule ngOnDestroy is not implemented raised by prefer-takeuntil rule when ngOnDestroy is in a super class Jun 24, 2020
@cartant
Copy link
Owner

cartant commented Jun 24, 2020

See this comment on the TSLint rule equivalent:

I imagine this would be possible, but complicated and I can't see myself having the time to do this anytime soon.

@cartant cartant added the enhancement New feature or request label Jun 24, 2020
@ldulary
Copy link
Author

ldulary commented Jun 25, 2020

Thanks for your answer.
Meanwhile I will just set checkDestroy to false.

@silveoj
Copy link

silveoj commented Feb 9, 2021

It's safer use

// eslint-disable-next-line rxjs-angular/prefer-takeuntil
export class MyComponent ...

or

  // eslint-disable-next-line rxjs-angular/prefer-takeuntil
  public ngOnDestroy() {
    super.ngOnDestroy();
  }

@Samuel-Therrien-Beslogic
Copy link

Samuel-Therrien-Beslogic commented Jan 18, 2022

See this comment on the TSLint rule equivalent:

I imagine this would be possible, but complicated and I can't see myself having the time to do this anytime soon.

For both prefer-composition and prefer-takeuntil,
May I suggest configuration to ignore if extended from a certain class? (and ngOnDestroy is not reimplemented in the child class). In my case I'd configure it for "BaseComponent". The rules would still naturally enforce ngOnDestroy and takeUntil for the BaseComponent anyway (since it doesn't extend itself).

@Component({
  selector: 'app-footer',
  templateUrl: './footer.component.html',
  styleUrls: ['./footer.component.less'],
})
export class FooterComponent extends BaseComponent implements OnInit {
  // Some code
}

Or even simpler as an easy / naive way to add support: let us disable the ngOnDestroy is not implemented message, while keeping the Forbids calling subscribe without an accompanying takeUntil one.

@cartant
Copy link
Owner

cartant commented Jan 26, 2022

@Samuel-Beslogic I just don't have the time to mess with these rules and I've not used them myself lately - as I've not used Angular for years. Rather than messing with them to work with base-class/component shenanigans, maybe the plain vanilla RxJS rules would be a better fit:

@Avasam
Copy link

Avasam commented Mar 11, 2022

@cartant Part of your rule does something important that neither of those two do: it ensures takeuntil used on subscriptions should properly unsubscribe when the component is destroyed.

It's just that there's part of the rule that wrongly assumes all components need to implement OnDestroy by themselves.

Even just splitting the takeUntil rule in two (or being able to disable the check for implementing OnDestroy) would be sufficient.
Forgetting takeUntil happens often. But when typing takeUntil(this.unsubscribe) it's not hard to see that I just need to extend our BaseComponent if Typescript tells me that this.unsubscribe does not exist.

I'm willing to spend some time on this and create a PR if I have a working solution. The AST Selector for the extend class would look something like this ClassDeclaration[superClass.name='${baseComponent}']

Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
Avasam referenced this issue in BesLogic/eslint-plugin-rxjs-angular May 21, 2022
@Avasam
Copy link

Avasam commented May 21, 2022

@ldulary #10
You can install github:Avasam/eslint-plugin-rxjs-angular#dist in the mean time with the following configuration:

"rxjs-angular/prefer-composition": [
  "error",
  {
    "superClass": [
      "BaseComponent"
    ]
  }
],
"rxjs-angular/prefer-takeuntil": [
  "error",
  {
    "superClass": [
      "BaseComponent"
    ]
  }
],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants