diff --git a/docs/rules/prefer-composition.md b/docs/rules/prefer-composition.md index e98dacb..210e54b 100644 --- a/docs/rules/prefer-composition.md +++ b/docs/rules/prefer-composition.md @@ -48,7 +48,11 @@ export class SomeComponent implements OnInit, OnDestroy { ## Options -This rule accepts a single option which is an object with a `checkDecorators` property which is an array containing the names of the decorators that determine whether or not a class is checked. By default, `checkDecorators` is `["Component"]`. +This rule accepts a single option which is an object with a `checkDecorators` and `superClass` properties. + +The `checkDecorators` property is an array containing the names of the decorators that determine whether or not a class is checked. By default, `checkDecorators` is `["Component"]`. + +The `superClass` property is an array containing the names of classes to extend from that already implements a `Subject`-based `ngOnDestroy`. ```json { @@ -61,4 +65,4 @@ This rule accepts a single option which is an object with a `checkDecorators` pr ## Further reading -- [Composing Subscriptions](https://ncjamieson.com/composing-subscriptions/) \ No newline at end of file +- [Composing Subscriptions](https://ncjamieson.com/composing-subscriptions/) diff --git a/docs/rules/prefer-takeuntil.md b/docs/rules/prefer-takeuntil.md index 2fc36b3..782a18e 100644 --- a/docs/rules/prefer-takeuntil.md +++ b/docs/rules/prefer-takeuntil.md @@ -50,12 +50,16 @@ class SomeComponent implements OnDestroy, OnInit { ## Options -This rule accepts a single option which is an object with `checkComplete`, `checkDecorators`, `checkDestroy` and `alias` properties. +This rule accepts a single option which is an object with `checkComplete`, `checkDecorators`, `checkDestroy`, `superClass` and `alias` properties. The `checkComplete` property is a boolean that determines whether or not `complete` must be called after `next` and the `checkDestroy` property is a boolean that determines whether or not a `Subject`-based `ngOnDestroy` must be implemented. The `checkDecorators` property is an array containing the names of the decorators that determine whether or not a class is checked. By default, `checkDecorators` is `["Component"]`. +The `checkDestroy` property is a boolean that determines whether or not a `Subject`-based `ngOnDestroy` must be implemented. + +The `superClass` property is an array containing the names of classes to extend from that already implements a `Subject`-based `ngOnDestroy`. + The `alias` property is an array of names of operators that should be treated similarly to `takeUntil`. ```json @@ -66,7 +70,8 @@ The `alias` property is an array of names of operators that should be treated si "alias": ["untilDestroyed"], "checkComplete": true, "checkDecorators": ["Component"], - "checkDestroy": true + "checkDestroy": true, + "superClass": [] } ] } @@ -74,4 +79,4 @@ The `alias` property is an array of names of operators that should be treated si ## Further reading -- [Avoiding takeUntil leaks](https://ncjamieson.com/avoiding-takeuntil-leaks/) \ No newline at end of file +- [Avoiding takeUntil leaks](https://ncjamieson.com/avoiding-takeuntil-leaks/) diff --git a/source/rules/prefer-composition.ts b/source/rules/prefer-composition.ts index 90a2962..6d159e1 100644 --- a/source/rules/prefer-composition.ts +++ b/source/rules/prefer-composition.ts @@ -18,6 +18,7 @@ import { ruleCreator } from "../utils"; const defaultOptions: readonly { checkDecorators?: string[]; + superClass?: string[]; }[] = []; const rule = ruleCreator({ @@ -40,11 +41,13 @@ const rule = ruleCreator({ { properties: { checkDecorators: { type: "array", items: { type: "string" } }, + superClass: { type: "array", items: { type: "string" } }, }, type: "object", description: stripIndent` An optional object with an optional \`checkDecorators\` property. The \`checkDecorators\` property is an array containing the names of the decorators that determine whether or not a class is checked. + The \`superClass\` property is an array containing the names of classes to extend from that already implements a \`Subject\`-based \`ngOnDestroy\`. `, }, ], @@ -53,7 +56,8 @@ const rule = ruleCreator({ name: "prefer-composition", create: (context, unused: typeof defaultOptions) => { const { couldBeObservable, couldBeSubscription } = getTypeServices(context); - const [{ checkDecorators = ["Component"] } = {}] = context.options; + const [{ checkDecorators = ["Component"], superClass = [] } = {}] = + context.options; type Entry = { addCallExpressions: es.CallExpression[]; @@ -61,6 +65,7 @@ const rule = ruleCreator({ propertyDefinitions: es.PropertyDefinition[]; hasDecorator: boolean; ngOnDestroyDefinition?: es.MethodDefinition; + extendsSuperClassDeclaration?: es.ClassDeclaration; subscribeCallExpressions: es.CallExpression[]; subscriptions: Set; unsubscribeCallExpressions: es.CallExpression[]; @@ -72,6 +77,7 @@ const rule = ruleCreator({ classDeclaration, propertyDefinitions, ngOnDestroyDefinition, + extendsSuperClassDeclaration, subscribeCallExpressions, subscriptions, unsubscribeCallExpressions, @@ -83,6 +89,9 @@ const rule = ruleCreator({ subscribeCallExpressions.forEach((callExpression) => { const { callee } = callExpression; if (isMemberExpression(callee)) { + if (extendsSuperClassDeclaration) { + return; + } const { object, property } = callee; if (!couldBeObservable(object)) { return; @@ -98,6 +107,9 @@ const rule = ruleCreator({ }); if (!ngOnDestroyDefinition) { + if (extendsSuperClassDeclaration) { + return; + } context.report({ messageId: "notImplemented", node: classDeclaration.id ?? classDeclaration, @@ -240,6 +252,20 @@ const rule = ruleCreator({ return true; } + const extendsSuperClassDeclaration = + superClass.length === 0 + ? {} + : { + [`ClassDeclaration:matches(${superClass + .map((className) => `[superClass.name="${className}"]`) + .join()})`]: (node: es.ClassDeclaration) => { + const entry = getEntry(); + if (entry && entry.hasDecorator) { + entry.extendsSuperClassDeclaration = node; + } + }, + }; + return { "CallExpression[callee.property.name='add']": ( node: es.CallExpression @@ -280,6 +306,7 @@ const rule = ruleCreator({ entry.propertyDefinitions.push(node); } }, + ...extendsSuperClassDeclaration, "MethodDefinition[key.name='ngOnDestroy'][kind='method']": ( node: es.MethodDefinition ) => { diff --git a/source/rules/prefer-takeuntil.ts b/source/rules/prefer-takeuntil.ts index f707665..8507a0a 100644 --- a/source/rules/prefer-takeuntil.ts +++ b/source/rules/prefer-takeuntil.ts @@ -26,11 +26,15 @@ const messages = { } as const; type MessageIds = keyof typeof messages; +const ngOnDestroyMethodSelector = + "MethodDefinition[key.name='ngOnDestroy'][kind='method']"; + const defaultOptions: readonly { alias?: string[]; checkComplete?: boolean; checkDecorators?: string[]; checkDestroy?: boolean; + superClass?: string[]; }[] = []; const rule = ruleCreator({ @@ -51,6 +55,7 @@ const rule = ruleCreator({ checkComplete: { type: "boolean" }, checkDecorators: { type: "array", items: { type: "string" } }, checkDestroy: { type: "boolean" }, + superClass: { type: "array", items: { type: "string" } }, }, type: "object", description: stripIndent` @@ -59,6 +64,7 @@ const rule = ruleCreator({ The \`checkComplete\` property is a boolean that determines whether or not \`complete\` must be called after \`next\`. The \`checkDecorators\` property is an array containing the names of the decorators that determine whether or not a class is checked. The \`checkDestroy\` property is a boolean that determines whether or not a \`Subject\`-based \`ngOnDestroy\` must be implemented. + The \`superClass\` property is an array containing the names of classes to extend from that already implements a \`Subject\`-based \`ngOnDestroy\`. `, }, ], @@ -78,6 +84,7 @@ const rule = ruleCreator({ checkComplete = false, checkDecorators = ["Component"], checkDestroy = alias.length === 0, + superClass = [], } = config; type Entry = { @@ -87,6 +94,8 @@ const rule = ruleCreator({ hasDecorator: boolean; nextCallExpressions: es.CallExpression[]; ngOnDestroyDefinition?: es.MethodDefinition; + extendsSuperClassDeclaration?: es.ClassDeclaration; + superNgOnDestroyCallExpression?: es.CallExpression; subscribeCallExpressions: es.CallExpression[]; subscribeCallExpressionsToNames: Map>; }; @@ -117,6 +126,8 @@ const rule = ruleCreator({ completeCallExpressions, nextCallExpressions, ngOnDestroyDefinition, + extendsSuperClassDeclaration, + superNgOnDestroyCallExpression, subscribeCallExpressionsToNames, } = entry; if (subscribeCallExpressionsToNames.size === 0) { @@ -124,6 +135,9 @@ const rule = ruleCreator({ } if (!ngOnDestroyDefinition) { + if (extendsSuperClassDeclaration) { + return; + } context.report({ messageId: "noDestroy", node: classDeclaration.id ?? classDeclaration, @@ -154,26 +168,39 @@ const rule = ruleCreator({ }; namesToChecks.set(name, check); - if (!checkSubjectProperty(name, entry)) { - check.descriptors.push({ - data: { name }, - messageId: "notDeclared", - node: classDeclaration.id ?? classDeclaration, - }); - } - if (!checkSubjectCall(name, nextCallExpressions)) { - check.descriptors.push({ - data: { method: "next", name }, - messageId: "notCalled", - node: ngOnDestroyDefinition.key, - }); - } - if (checkComplete && !checkSubjectCall(name, completeCallExpressions)) { - check.descriptors.push({ - data: { method: "complete", name }, - messageId: "notCalled", - node: ngOnDestroyDefinition.key, - }); + if (extendsSuperClassDeclaration) { + if (!superNgOnDestroyCallExpression) { + check.descriptors.push({ + data: { method: "ngOnDestroy", name: "super" }, + messageId: "notCalled", + node: ngOnDestroyDefinition.key, + }); + } + } else { + if (!checkSubjectProperty(name, entry)) { + check.descriptors.push({ + data: { name }, + messageId: "notDeclared", + node: classDeclaration.id ?? classDeclaration, + }); + } + if (!checkSubjectCall(name, nextCallExpressions)) { + check.descriptors.push({ + data: { method: "next", name }, + messageId: "notCalled", + node: ngOnDestroyDefinition.key, + }); + } + if ( + checkComplete && + !checkSubjectCall(name, completeCallExpressions) + ) { + check.descriptors.push({ + data: { method: "complete", name }, + messageId: "notCalled", + node: ngOnDestroyDefinition.key, + }); + } } }); @@ -308,6 +335,20 @@ const rule = ruleCreator({ ); } + const extendsSuperClassDeclaration = + superClass.length === 0 + ? {} + : { + [`ClassDeclaration:matches(${superClass + .map((className) => `[superClass.name="${className}"]`) + .join()})`]: (node: es.ClassDeclaration) => { + const entry = getEntry(); + if (entry && entry.hasDecorator) { + entry.extendsSuperClassDeclaration = node; + } + }, + }; + return { "CallExpression[callee.property.name='subscribe']": ( node: es.CallExpression @@ -344,22 +385,28 @@ const rule = ruleCreator({ entry.propertyDefinitions.push(node); } }, - "MethodDefinition[key.name='ngOnDestroy'][kind='method']": ( - node: es.MethodDefinition - ) => { + [ngOnDestroyMethodSelector]: (node: es.MethodDefinition) => { const entry = getEntry(); if (entry && entry.hasDecorator) { entry.ngOnDestroyDefinition = node; } }, - "MethodDefinition[key.name='ngOnDestroy'][kind='method'] CallExpression[callee.property.name='next']": + ...extendsSuperClassDeclaration, + [`${ngOnDestroyMethodSelector} CallExpression[callee.object.type='Super'][callee.property.name='ngOnDestroy']`]: + (node: es.CallExpression) => { + const entry = getEntry(); + if (entry && entry.hasDecorator) { + entry.superNgOnDestroyCallExpression = node; + } + }, + [`${ngOnDestroyMethodSelector} CallExpression[callee.property.name='next']`]: (node: es.CallExpression) => { const entry = getEntry(); if (entry && entry.hasDecorator) { entry.nextCallExpressions.push(node); } }, - "MethodDefinition[key.name='ngOnDestroy'][kind='method'] CallExpression[callee.property.name='complete']": + [`${ngOnDestroyMethodSelector} CallExpression[callee.property.name='complete']`]: (node: es.CallExpression) => { const entry = getEntry(); if (entry && entry.hasDecorator) { diff --git a/tests/rules/prefer-composition.ts b/tests/rules/prefer-composition.ts index 7084a8e..7af6f83 100644 --- a/tests/rules/prefer-composition.ts +++ b/tests/rules/prefer-composition.ts @@ -98,6 +98,44 @@ ruleTester({ types: true }).run("prefer-composition", rule, { } `, }, + { + code: stripIndent` + // extends superClass + // https://github.com/cartant/eslint-plugin-rxjs-angular/issues/1 + import { Component, Directive, OnDestroy } from "@angular/core"; + import { of, Subject } from "rxjs"; + import { switchMap, takeUntil } from "rxjs/operators"; + + const o = of("o"); + + @Directive() + abstract class BaseComponent implements OnDestroy { + private readonly destroySubject = new Subject(); + protected readonly destroy = this.destroySubject.asObservable(); + ngOnDestroy() { + this.destroySubject.next(); + this.destroySubject.complete(); + } + } + + @Component({ + selector: "component-with-super-class" + }) + class CorrectComponent extends BaseComponent { + someMethod() { + o.pipe( + switchMap(_ => o), + takeUntil(this.destroy) + ).subscribe(); + } + } + `, + options: [ + { + superClass: ["BaseComponent"], + }, + ], + }, ], invalid: [ fromFixture( diff --git a/tests/rules/prefer-takeuntil.ts b/tests/rules/prefer-takeuntil.ts index 1a6b2d2..e12b35d 100644 --- a/tests/rules/prefer-takeuntil.ts +++ b/tests/rules/prefer-takeuntil.ts @@ -369,6 +369,90 @@ ruleTester({ types: true }).run("prefer-takeuntil", rule, { }, ], }, + { + code: stripIndent` + // extends superClass + // https://github.com/cartant/eslint-plugin-rxjs-angular/issues/1 + import { Component, Directive, OnDestroy } from "@angular/core"; + import { of, Subject } from "rxjs"; + import { switchMap, takeUntil } from "rxjs/operators"; + + const o = of("o"); + + @Directive() + abstract class BaseComponent implements OnDestroy { + private readonly destroySubject = new Subject(); + protected readonly destroy = this.destroySubject.asObservable(); + + ngOnDestroy() { + this.destroySubject.next(); + this.destroySubject.complete(); + } + } + + @Component({ + selector: "component-with-super-class" + }) + class CorrectComponent extends BaseComponent { + someMethod() { + o.pipe( + switchMap(_ => o), + takeUntil(this.destroy) + ).subscribe(); + } + } + `, + options: [ + { + superClass: ["BaseComponent"], + checkComplete: true, + }, + ], + }, + { + code: stripIndent` + // extends superClass and implements OnDestroy + // https://github.com/cartant/eslint-plugin-rxjs-angular/issues/1 + import { Component, Directive, OnDestroy } from "@angular/core"; + import { of, Subject } from "rxjs"; + import { switchMap, takeUntil } from "rxjs/operators"; + + const o = of("o"); + + @Directive() + abstract class BaseComponent implements OnDestroy { + private readonly destroySubject = new Subject(); + protected readonly destroy = this.destroySubject.asObservable(); + + ngOnDestroy() { + this.destroySubject.next(); + this.destroySubject.complete(); + } + } + + @Component({ + selector: "component-with-super-class-and-destroy" + }) + class CorrectDestroyComponent extends BaseComponent implements OnDestroy { + someMethod() { + o.pipe( + switchMap(_ => o), + takeUntil(this.destroy) + ).subscribe(); + } + + override ngOnDestroy(): void { + super.ngOnDestroy() + } + } + `, + options: [ + { + superClass: ["BaseComponent"], + checkComplete: true, + }, + ], + }, ], invalid: [ fromFixture( @@ -657,5 +741,89 @@ ruleTester({ types: true }).run("prefer-takeuntil", rule, { ], } ), + fromFixture( + stripIndent` + // extends superClass and implements OnDestroy, missing super.ngOnDestroy() + // https://github.com/cartant/eslint-plugin-rxjs-angular/issues/1 + import { Component, Directive, OnDestroy } from "@angular/core"; + import { of, Subject } from "rxjs"; + import { switchMap, takeUntil } from "rxjs/operators"; + + const o = of("o"); + + @Directive() + abstract class BaseComponent implements OnDestroy { + private readonly destroySubject = new Subject(); + protected readonly destroy = this.destroySubject.asObservable(); + + ngOnDestroy() { + this.destroySubject.next(); + this.destroySubject.complete(); + } + } + + @Component({ + selector: "missing-super-call" + }) + class MissingSuperCallComponent extends BaseComponent implements OnDestroy { + someMethod() { + o.pipe( + switchMap(_ => o), + takeUntil(this.destroy) + ).subscribe(); + } + + override ngOnDestroy(): void { + ~~~~~~~~~~~ [notCalled { "method": "ngOnDestroy", "name": "super" }] + } + } + `, + { + options: [ + { + superClass: ["BaseComponent"], + checkComplete: true, + }, + ], + } + ), + fromFixture( + stripIndent` + // Calls super.ngOnDestroy() w/o extending base class + // https://github.com/cartant/eslint-plugin-rxjs-angular/issues/1 + import { Component, OnDestroy } from "@angular/core"; + import { of } from "rxjs"; + import { switchMap, takeUntil } from "rxjs/operators"; + + const o = of("o"); + + @Component({ + selector: "missing-base" + }) + class MissingBaseComponent extends SomeClass implements OnDestroy { + ~~~~~~~~~~~~~~~~~~~~ [notDeclared { "name": "destroy" }] + someMethod() { + o.pipe( + switchMap(_ => o), + takeUntil(this.destroy) + ).subscribe(); + } + + override ngOnDestroy(): void { + ~~~~~~~~~~~ [notCalled { "method": "next", "name": "destroy" }] + ~~~~~~~~~~~ [notCalled { "method": "complete", "name": "destroy" }] + super.ngOnDestroy() + } + } + `, + { + options: [ + { + superClass: ["BaseComponent"], + checkComplete: true, + }, + ], + } + ), ], });