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

Added superClass option. Fixes #1 #10

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Avasam
Copy link

@Avasam Avasam commented May 21, 2022

Updated documentation and tests (I noticed checkDestroy was missing from the readme as well). Ran the linter and did my best to follow what seemed to be the code style.

#1 (comment)

Adds the option to specify a class that components extends from which already implements the unsubscribe ngOnDestroy pattern.
Since the super class won't extend itself, this keeps everything safe. I've also considered checking for any super class by default (same logic where if the super class doesn't implement the pattern, linting would fail, just in a different location). But decided against it in the end, since someone could extend a class they don't have control over, or isn't linted. If you think it would be useful, I can add that functionality.

@Samuel-Therrien-Beslogic
Copy link

Samuel-Therrien-Beslogic commented May 22, 2022

Note this doesn't detect calls to super.ngOnDestroy() inside a sub-component's own ngOnDestroy. And will hence raise the errors unsubscribe.next() not called. and Subject unsubscribe not a class property. for rule rxjs-angular/prefer-takeuntil. It shouldn't be too complicated to add support for it using the exact same strategy.

@Samuel-Therrien-Beslogic

@cartant Could you take a look at this? I don't think anything's missing actually.

@Samuel-Therrien-Beslogic
Copy link

Samuel-Therrien-Beslogic commented Apr 11, 2024

I was given some time at work to finish this PR to resolve a handful of false positives in our projects as per my previous comment.

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

Successfully merging this pull request may close these issues.

2 participants