From 68c668040c9cf399e8f49b1a46731d8244116863 Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Wed, 6 Nov 2019 15:12:40 +0100 Subject: [PATCH 1/6] feat: add rxjs-prefer-angular-takeuntil-before-subscribe rule --- .gitignore | 1 + README.md | 54 +++ docs/index.md | 54 +++ ...erAngularTakeuntilBeforeUnsubscribeRule.ts | 352 ++++++++++++++++++ .../default/fixture.ts.lint | 88 +++++ .../default/tsconfig.json | 13 + .../default/tslint.json | 8 + 7 files changed, 570 insertions(+) create mode 100644 source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts create mode 100644 test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint create mode 100644 test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json create mode 100644 test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json diff --git a/.gitignore b/.gitignore index 248d183b..dbcd095a 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ yarn-error.log /dist /node_modules /temp +/.idea diff --git a/README.md b/README.md index f9fc00be..6ec51c09 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,7 @@ The package includes the following rules (none of which are enabled by default): | `rxjs-no-unsafe-takeuntil` | Disallows the application of operators after `takeUntil`. Operators placed after `takeUntil` can effect [subscription leaks](https://medium.com/@cartant/rxjs-avoiding-takeuntil-leaks-fb5182d047ef). | [See below](#rxjs-no-unsafe-takeuntil) | | `rxjs-no-unused-add` | Disallows the importation of patched observables or operators that are not used in the module. | None | | `rxjs-no-wholesale` | Disallows the wholesale importation of `rxjs` or `rxjs/Rx`. | None | +| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-angular-prefer-takeuntil-before-subscribe) | | `rxjs-prefer-angular-async-pipe` | Disallows the calling of `subscribe` within an Angular component. | None | | `rxjs-prefer-observer` | Enforces the passing of observers to `subscribe` and `tap`. See [this RxJS issue](https://github.com/ReactiveX/rxjs/issues/4159). | [See below](#rxjs-prefer-observer) | | `rxjs-suffix-subjects` | Disalllows subjects that don't end with the specified `suffix` option. | [See below](#rxjs-suffix-subjects) | @@ -395,6 +396,59 @@ The following options are equivalent to the rule's default configuration: } ``` + + +#### rxjs-angular-prefer-takeuntil-before-subscribe + +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()` +and ensuring the component implements the `ngOnDestroy` +method invoking `this.destroy$.next()` and `this.destroy$.complete()`. + +The rule takes an optional object with an optional `allowedDestroySubjectNames` property, +which is an array containing the allowed subject names expected as argument of `takeUntil`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$']. + +The following options are equivalent to the rule's default configuration: + +```json +"rules": { + "rxjs-angular-prefer-takeuntil-before-subscribe": { + "options": [{ + "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] + }], + "severity": "error" + } +} +``` +##### Example +This should trigger an error: +```typescript + +class MyComponent { +// ~~~~~~~~~~~ component containing subscribe must implement the ngOnDestroy() method + someMethod() { + const e = a.pipe(switchMap(_ => b)).subscribe(); +// ~~~~~~~~~ subscribe within a component must be preceded by takeUntil + } +} +``` + +while this should be fine: +```typescript +class MyComponent implements SomeInterface, OnDestroy { + private destroy$: Subject = new Subject(); + + someMethod() { + const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); + } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } +} +``` + #### rxjs-prefer-observer diff --git a/docs/index.md b/docs/index.md index b4931bfd..c12adbb5 100644 --- a/docs/index.md +++ b/docs/index.md @@ -53,6 +53,7 @@ The package includes the following rules (none of which are enabled by default): | `rxjs-no-unsafe-takeuntil` | Disallows the application of operators after `takeUntil`. Operators placed after `takeUntil` can effect [subscription leaks](https://medium.com/@cartant/rxjs-avoiding-takeuntil-leaks-fb5182d047ef). | [See below](#rxjs-no-unsafe-takeuntil) | | `rxjs-no-unused-add` | Disallows the importation of patched observables or operators that are not used in the module. | None | | `rxjs-no-wholesale` | Disallows the wholesale importation of `rxjs` or `rxjs/Rx`. | None | +| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-angular-prefer-takeuntil-before-subscribe) | | `rxjs-prefer-angular-async-pipe` | Disallows the calling of `subscribe` within an Angular component. | None | | `rxjs-prefer-observer` | Enforces the passing of observers to `subscribe` and `tap`. See [this RxJS issue](https://github.com/ReactiveX/rxjs/issues/4159). | [See below](#rxjs-prefer-observer) | | `rxjs-suffix-subjects` | Disalllows subjects that don't end with the specified `suffix` option. | [See below](#rxjs-suffix-subjects) | @@ -331,6 +332,59 @@ The following options are equivalent to the rule's default configuration: } ``` + + +#### rxjs-angular-prefer-takeuntil-before-subscribe + +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()` +and ensuring the component implements the `ngOnDestroy` +method invoking `this.destroy$.next()` and `this.destroy$.complete()`. + +The rule takes an optional object with an optional `allowedDestroySubjectNames` property, +which is an array containing the allowed subject names expected as argument of `takeUntil`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$']. + +The following options are equivalent to the rule's default configuration: + +```json +"rules": { + "rxjs-angular-prefer-takeuntil-before-subscribe": { + "options": [{ + "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] + }], + "severity": "error" + } +} +``` +##### Example +This should trigger an error: +```typescript + +class MyComponent { +// ~~~~~~~~~~~ component containing subscribe must implement the ngOnDestroy() method + someMethod() { + const e = a.pipe(switchMap(_ => b)).subscribe(); +// ~~~~~~~~~ subscribe within a component must be preceded by takeUntil + } +} +``` + +while this should be fine: +```typescript +class MyComponent implements SomeInterface, OnDestroy { + private destroy$: Subject = new Subject(); + + someMethod() { + const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); + } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } +} +``` + #### rxjs-prefer-observer diff --git a/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts b/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts new file mode 100644 index 00000000..43a4822b --- /dev/null +++ b/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts @@ -0,0 +1,352 @@ +/** + * @license Use of this source code is governed by an MIT-style license that + * can be found in the LICENSE file at https://github.com/cartant/rxjs-tslint-rules + */ +/*tslint:disable:no-use-before-declare*/ + +import * as Lint from "tslint"; +import * as tsutils from "tsutils"; +import * as ts from "typescript"; +import { couldBeType } from "../support/util"; +import { tsquery } from "@phenomnomnominal/tsquery"; +import { dedent } from "tslint/lib/utils"; + +export class Rule extends Lint.Rules.TypedRule { + public static metadata: Lint.IRuleMetadata = { + description: dedent`Enforces the application of the takeUntil operator + when calling of subscribe within an Angular component.`, + options: { + properties: { + allowedDestroySubjectNames: { type: "array", items: { type: "string" } } + }, + type: "object" + }, + optionsDescription: Lint.Utils.dedent` + An optional object with optional \`destroySubjectNames\` property. + The property is an array containing the allowed subject names expected as argument of \`takeUntil\`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$'].`, + requiresTypeInfo: true, + ruleName: "rxjs-prefer-angular-takeuntil-before-unsubscribe", + type: "functionality", + typescriptOnly: true + }; + + public static FAILURE_STRING = + "subscribe within a component must be preceded by takeUntil"; + + public static FAILURE_STRING_SUBJECT_NAME = + "takeUntil argument must be one of {allowedDestroySubjectNames}"; + + public static FAILURE_STRING_NG_ON_DESTROY = + "component containing subscribe must implement the ngOnDestroy() method"; + + public static FAILURE_STRING_NG_ON_DESTROY_SUBJECT_METHOD_NOT_CALLED = + "there must be an invocation of {destroySubjectName}.{methodName}() in ngOnDestroy()"; + + private allowedDestroySubjectNames: string[] = ["destroy$", "_destroy$"]; + + public applyWithProgram( + sourceFile: ts.SourceFile, + program: ts.Program + ): Lint.RuleFailure[] { + const failures: Lint.RuleFailure[] = []; + + // initialize the options + const options = this.getOptions(); + if (options.ruleArguments) { + const allowedDestroySubjectNamesOption = options.ruleArguments.find( + ruleArgument => + ruleArgument.hasOwnProperty("allowedDestroySubjectNames") + ); + if ( + allowedDestroySubjectNamesOption && + Array.isArray( + allowedDestroySubjectNamesOption["allowedDestroySubjectNames"] + ) + ) { + this.allowedDestroySubjectNames = + allowedDestroySubjectNamesOption["allowedDestroySubjectNames"]; + } + } + + // find all classes with an @Component() decorator + const componentClassDeclarations = tsquery( + sourceFile, + `ClassDeclaration:has(Decorator[expression.expression.name='Component'])` + ); + componentClassDeclarations.forEach(componentClassDeclaration => { + failures.push( + ...this.checkComponentClassDeclaration( + sourceFile, + program, + componentClassDeclaration as ts.ClassDeclaration + ) + ); + }); + + return failures; + } + + /** + * Checks a component class for occurrences of .subscribe() and corresponding takeUntil() requirements + */ + private checkComponentClassDeclaration( + sourceFile: ts.SourceFile, + program: ts.Program, + componentClassDeclaration: ts.ClassDeclaration + ): Lint.RuleFailure[] { + const failures: Lint.RuleFailure[] = []; + + const typeChecker = program.getTypeChecker(); + /** whether there is at least one observable.subscribe() expression */ + let hasSubscribeInComponent = false; + /** list of destroy subjects used in takeUntil() operators */ + const destroySubjectNamesUsed: string[] = []; + + // find observable.subscribe() call expressions + const propertyAccessExpressions = tsquery( + componentClassDeclaration, + `CallExpression > PropertyAccessExpression[name.name="subscribe"]` + ); + + // check whether it is an observable and check the takeUntil before the subscribe + propertyAccessExpressions.forEach(node => { + const propertyAccessExpression = node as ts.PropertyAccessExpression; + const type = typeChecker.getTypeAtLocation( + propertyAccessExpression.expression + ); + if (couldBeType(type, "Observable")) { + const subscribeFailures = this.ensureTakeuntilBeforeSubscribe( + sourceFile, + propertyAccessExpression + ); + failures.push(...subscribeFailures.failures); + if ( + subscribeFailures.destroySubjectName && + !destroySubjectNamesUsed.includes( + subscribeFailures.destroySubjectName + ) + ) { + destroySubjectNamesUsed.push(subscribeFailures.destroySubjectName); + } + hasSubscribeInComponent = true; + } + }); + + // check the ngOnDestroyMethod + if (hasSubscribeInComponent) { + const ngOnDestroyFailures = this.checkNgOnDestroy( + sourceFile, + componentClassDeclaration as ts.ClassDeclaration, + destroySubjectNamesUsed + ); + failures.push(...ngOnDestroyFailures); + } + + return failures; + } + + /** + * Checks whether a .subscribe() is preceded by a .pipe(<...>, takeUntil(<...>)) + */ + private ensureTakeuntilBeforeSubscribe( + sourceFile: ts.SourceFile, + node: ts.PropertyAccessExpression + ): { failures: Lint.RuleFailure[]; destroySubjectName: string } { + const failures: Lint.RuleFailure[] = []; + const subscribeContext = node.expression; + + /** Whether a takeUntil() operator preceding the .subscribe() was found */ + let lastTakeUntilFound = false; + /** name of the takeUntil() argument */ + let destroySubjectName: string; + + // check whether subscribeContext.expression is .pipe() + if ( + tsutils.isCallExpression(subscribeContext) && + tsutils.isPropertyAccessExpression(subscribeContext.expression) && + subscribeContext.expression.name.getText() === "pipe" + ) { + const pipedOperators = subscribeContext.arguments; + if (pipedOperators.length > 0) { + const lastPipedOperator = pipedOperators[pipedOperators.length - 1]; + // check whether the last operator in the .pipe() call is takeUntil() + if ( + tsutils.isCallExpression(lastPipedOperator) && + tsutils.isIdentifier(lastPipedOperator.expression) && + lastPipedOperator.expression.text === "takeUntil" + ) { + lastTakeUntilFound = true; + // check the argument of takeUntil() + const destroySubjectNameCheck = this.checkDestroySubjectName( + sourceFile, + lastPipedOperator + ); + failures.push(...destroySubjectNameCheck.failures); + destroySubjectName = destroySubjectNameCheck.destroySubjectName; + } + } + } + + // add failure if there is no takeUntil() in the last position of a .pipe() + if (!lastTakeUntilFound) { + failures.push( + new Lint.RuleFailure( + sourceFile, + node.name.getStart(), + node.name.getStart() + node.name.getWidth(), + Rule.FAILURE_STRING, + this.ruleName + ) + ); + } + + return { failures, destroySubjectName: destroySubjectName }; + } + + /** + * Checks whether the argument of the given takeUntil(this.destroy$) expression + * is among the list of allowedDestroySubjectNames + */ + private checkDestroySubjectName( + sourceFile: ts.SourceFile, + takeUntilOperator: ts.CallExpression + ): { failures: Lint.RuleFailure[]; destroySubjectName: string } { + const failures: Lint.RuleFailure[] = []; + + /** name of the takeUntil() argument */ + let destroySubjectName: string; + + /** whether the takeUntil() argument is among the allowed names */ + let isAllowedDestroySubject = false; + + let takeUntilOperatorArgument: ts.PropertyAccessExpression; + let highlightedNode: ts.Expression = takeUntilOperator; + + // check the takeUntil() argument + if ( + takeUntilOperator.arguments.length >= 1 && + takeUntilOperator.arguments[0] + ) { + highlightedNode = takeUntilOperator.arguments[0]; + if (tsutils.isPropertyAccessExpression(takeUntilOperator.arguments[0])) { + takeUntilOperatorArgument = takeUntilOperator + .arguments[0] as ts.PropertyAccessExpression; + destroySubjectName = takeUntilOperatorArgument.name.getText(); + + isAllowedDestroySubject = this.allowedDestroySubjectNames.some( + allowedDestroySubjectName => + takeUntilOperatorArgument.name.getText() === + allowedDestroySubjectName + ); + } + } + + if (!isAllowedDestroySubject) { + failures.push( + new Lint.RuleFailure( + sourceFile, + highlightedNode.getStart(), + highlightedNode.getStart() + highlightedNode.getWidth(), + Rule.FAILURE_STRING_SUBJECT_NAME.replace( + "{allowedDestroySubjectNames}", + "[" + + this.allowedDestroySubjectNames + .map(name => "this." + name) + .join(", ") + + "]" + ), + this.ruleName + ) + ); + } + + return { failures, destroySubjectName }; + } + + /** + * Checks whether the class implements an ngOnDestroy method and invokes .next() and .complete() on the destroy subjects + */ + private checkNgOnDestroy( + sourceFile: ts.SourceFile, + classDeclaration: ts.ClassDeclaration, + destroySubjectNamesUsed: string[] + ): Lint.RuleFailure[] { + const failures: Lint.RuleFailure[] = []; + const ngOnDestroyMethod = classDeclaration.members.find( + member => member.name.getText() === "ngOnDestroy" + ); + + // check whether the ngOnDestroy method is implemented + // and contains invocations of .next() and .complete() on all destroy subjects used + if (ngOnDestroyMethod) { + failures.push( + ...this.checkDestroySubjectMethodInvocation( + sourceFile, + ngOnDestroyMethod, + destroySubjectNamesUsed, + "next" + ) + ); + failures.push( + ...this.checkDestroySubjectMethodInvocation( + sourceFile, + ngOnDestroyMethod, + destroySubjectNamesUsed, + "complete" + ) + ); + } else { + failures.push( + new Lint.RuleFailure( + sourceFile, + classDeclaration.name.getStart(), + classDeclaration.name.getStart() + classDeclaration.name.getWidth(), + Rule.FAILURE_STRING_NG_ON_DESTROY, + this.ruleName + ) + ); + } + return failures; + } + + /** + * Checks whether all >destroySubjectNameUsed>.() are invoked in the ngOnDestroyMethod + */ + private checkDestroySubjectMethodInvocation( + sourceFile: ts.SourceFile, + ngOnDestroyMethod: ts.ClassElement, + destroySubjectNamesUsed: string[], + methodName: string + ) { + const failures: Lint.RuleFailure[] = []; + const destroySubjectMethodInvocations = tsquery( + ngOnDestroyMethod, + `CallExpression > PropertyAccessExpression[name.name="${methodName}"]` + ) as ts.PropertyAccessExpression[]; + destroySubjectNamesUsed.forEach(destroySubjectName => { + // check whether there is one invocation of .() + if ( + !destroySubjectMethodInvocations.some( + nextInvocation => + tsutils.isPropertyAccessExpression(nextInvocation.expression) && + nextInvocation.expression.name.getText() === destroySubjectName + ) + ) { + failures.push( + new Lint.RuleFailure( + sourceFile, + ngOnDestroyMethod.name.getStart(), + ngOnDestroyMethod.name.getStart() + + ngOnDestroyMethod.name.getWidth(), + Rule.FAILURE_STRING_NG_ON_DESTROY_SUBJECT_METHOD_NOT_CALLED.replace( + "{destroySubjectName}", + `this.${destroySubjectName}` + ).replace("{methodName}", methodName), + this.ruleName + ) + ); + } + }); + return failures; + } +} diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint new file mode 100644 index 00000000..6b199c9a --- /dev/null +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint @@ -0,0 +1,88 @@ +import { combineLatest, of, Subject } from "rxjs"; +import { switchMap, takeUntil } from "rxjs/operators"; + +const a = of("a"); +const b = of("b"); +const c = of("c"); +const d = of("d"); + +const e = a.pipe(switchMap(_ => b)).subscribe(); + +const f = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); + +const g = a.pipe(takeUntil(d), s => switchMap(_ => b)).subscribe(); + +class MyClass { + someMethod() { + const e = a.pipe(switchMap(_ => b)).subscribe(); + + const f = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); + + const g = a.pipe(takeUntil(d), s => switchMap(_ => b)).subscribe(); + } +} + +@Component({ + selector: 'app-my' +}) +class MyComponent { + ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-ondestroy] + + private destroy$: Subject = new Subject(); + + someMethod() { + const d = a.subscribe(); + ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + + const e = a.pipe(switchMap(_ => b)).subscribe(); + ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + + const f = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); + + const g = a.pipe(takeUntil(d), s => switchMap(_ => b)).subscribe(); + ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + + const f = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); + ~ [enforce-takeuntil-before-unsubscribe-subject-name] + } +} + +@Component({ + selector: 'app-my' +}) +class MyComponent implements OnDestroy { + someMethod() { + const f = a.pipe(switchMap(_ => b), takeUntil(this._destroy$)).subscribe(); + } + + ngOnDestroy() { + ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-next-missing] + ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-complete-missing] + // this._destroy$.next() is missing + this.destroy$.next(); + this.destroy$.complete(); + } +} + +@Component({ + selector: 'app-my' +}) +class MyComponent implements SomeInterface, OnDestroy { + private destroy$: Subject = new Subject(); + + someMethod() { + const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); + } + + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } +} + + +[enforce-takeuntil-before-unsubscribe]: subscribe within a component must be preceded by takeUntil +[enforce-takeuntil-before-unsubscribe-subject-name]: takeUntil argument must be one of [this.destroy$, this._destroy$] +[enforce-takeuntil-before-unsubscribe-ondestroy]: component containing subscribe must implement the ngOnDestroy() method +[enforce-takeuntil-before-unsubscribe-next-missing]: there must be an invocation of this._destroy$.next() in ngOnDestroy() +[enforce-takeuntil-before-unsubscribe-complete-missing]: there must be an invocation of this._destroy$.complete() in ngOnDestroy() diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json new file mode 100644 index 00000000..690be78e --- /dev/null +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json @@ -0,0 +1,13 @@ +{ + "compilerOptions": { + "baseUrl": ".", + "lib": ["es2015"], + "noEmit": true, + "paths": { + "rxjs": ["../../node_modules/rxjs"] + }, + "skipLibCheck": true, + "target": "es5" + }, + "include": ["fixture.ts"] +} diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json new file mode 100644 index 00000000..3b91e898 --- /dev/null +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json @@ -0,0 +1,8 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-prefer-angular-takeuntil-before-unsubscribe": { "severity": "error" } + }, + "rulesDirectory": "../../../../../build/rules" +} From eebee7ec85806ecd5be7d00d08f1637887b74b73 Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Fri, 8 Nov 2019 10:58:04 +0100 Subject: [PATCH 2/6] feat: improve rxjs-prefer-angular-takeuntil-before-subscribe rule check takeuntil before shareReplay, etc. --- README.md | 8 + docs/index.md | 8 + package.json | 1 + ...erAngularTakeuntilBeforeUnsubscribeRule.ts | 176 +++++++++++++++--- .../default/fixture.ts.lint | 29 ++- 5 files changed, 194 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 6ec51c09..6da078b8 100644 --- a/README.md +++ b/README.md @@ -402,6 +402,7 @@ The following options are equivalent to the rule's default configuration: 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()`. @@ -426,6 +427,11 @@ This should trigger an error: 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 @@ -438,6 +444,8 @@ while this should be fine: class MyComponent implements SomeInterface, OnDestroy { private destroy$: Subject = new Subject(); + k$ = a.pipe(takeUntil(this.destroy$), shareReplay(1)); + someMethod() { const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); } diff --git a/docs/index.md b/docs/index.md index c12adbb5..028f47c4 100644 --- a/docs/index.md +++ b/docs/index.md @@ -338,6 +338,7 @@ The following options are equivalent to the rule's default configuration: 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()`. @@ -362,6 +363,11 @@ This should trigger an error: 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 @@ -374,6 +380,8 @@ while this should be fine: class MyComponent implements SomeInterface, OnDestroy { private destroy$: Subject = new Subject(); + k$ = a.pipe(takeUntil(this.destroy$), shareReplay(1)); + someMethod() { const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); } diff --git a/package.json b/package.json index 8e1273b6..5432c9df 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,7 @@ "test:mocha": "mocha \"./build/**/*-spec.js\"", "test:tslint-v5": "yarn --cwd ./test/v5 install && yarn --cwd ./test/v5 upgrade && tslint --test \"./test/v5/fixtures/**/tslint.json\"", "test:tslint-v6": "yarn --cwd ./test/v6 install && yarn --cwd ./test/v6 upgrade && tslint --test \"./test/v6/fixtures/**/tslint.json\"", + "special-takeuntil": "npm run test:build && tslint --test \"test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/**/tslint.json\"", "test:tslint-v6-compat": "yarn --cwd ./test/v6-compat install && yarn --cwd ./test/v6-compat upgrade && tslint --test \"./test/v6-compat/fixtures/**/tslint.json\"" }, "version": "4.26.1" diff --git a/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts b/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts index 43a4822b..830beaa4 100644 --- a/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts +++ b/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts @@ -36,6 +36,9 @@ export class Rule extends Lint.Rules.TypedRule { public static FAILURE_STRING_SUBJECT_NAME = "takeUntil argument must be one of {allowedDestroySubjectNames}"; + public static FAILURE_STRING_OPERATOR = + "the {operator} operator used within a component must be preceded by takeUntil"; + public static FAILURE_STRING_NG_ON_DESTROY = "component containing subscribe must implement the ngOnDestroy() method"; @@ -44,6 +47,14 @@ export class Rule extends Lint.Rules.TypedRule { private allowedDestroySubjectNames: string[] = ["destroy$", "_destroy$"]; + private operatorsRequiringPrecedingTakeuntil: string[] = [ + "publish", + "publishBehavior", + "publishLast", + "publishReplay", + "shareReplay" + ]; + public applyWithProgram( sourceFile: ts.SourceFile, program: ts.Program @@ -100,44 +111,69 @@ export class Rule extends Lint.Rules.TypedRule { /** whether there is at least one observable.subscribe() expression */ let hasSubscribeInComponent = false; /** list of destroy subjects used in takeUntil() operators */ - const destroySubjectNamesUsed: string[] = []; + const destroySubjectNamesUsed: { + [destroySubjectName: string]: boolean; + } = {}; // find observable.subscribe() call expressions - const propertyAccessExpressions = tsquery( + const subscribePropertyAccessExpressions = tsquery( componentClassDeclaration, `CallExpression > PropertyAccessExpression[name.name="subscribe"]` ); // check whether it is an observable and check the takeUntil before the subscribe - propertyAccessExpressions.forEach(node => { + subscribePropertyAccessExpressions.forEach(node => { const propertyAccessExpression = node as ts.PropertyAccessExpression; const type = typeChecker.getTypeAtLocation( propertyAccessExpression.expression ); if (couldBeType(type, "Observable")) { - const subscribeFailures = this.ensureTakeuntilBeforeSubscribe( + const subscribeFailures = this.checkTakeuntilBeforeSubscribe( sourceFile, propertyAccessExpression ); failures.push(...subscribeFailures.failures); - if ( - subscribeFailures.destroySubjectName && - !destroySubjectNamesUsed.includes( - subscribeFailures.destroySubjectName - ) - ) { - destroySubjectNamesUsed.push(subscribeFailures.destroySubjectName); + if (subscribeFailures.destroySubjectName) { + destroySubjectNamesUsed[subscribeFailures.destroySubjectName] = true; } hasSubscribeInComponent = true; } }); + // find observable.pipe() call expressions + const pipePropertyAccessExpressions = tsquery( + componentClassDeclaration, + `CallExpression > PropertyAccessExpression[name.name="pipe"]` + ); + + // check whether it is an observable and check the takeUntil before operators requiring it + pipePropertyAccessExpressions.forEach(node => { + const propertyAccessExpression = node as ts.PropertyAccessExpression; + const pipeCallExpression = node.parent as ts.CallExpression; + const type = typeChecker.getTypeAtLocation( + propertyAccessExpression.expression + ); + if (couldBeType(type, "Observable")) { + const pipeFailures = this.checkTakeuntilBeforeOperatorsInPipe( + sourceFile, + pipeCallExpression.arguments + ); + failures.push(...pipeFailures.failures); + pipeFailures.destroySubjectNames.forEach(destroySubjectName => { + if (destroySubjectName) { + destroySubjectNamesUsed[destroySubjectName] = true; + } + }); + hasSubscribeInComponent = true; + } + }); + // check the ngOnDestroyMethod if (hasSubscribeInComponent) { const ngOnDestroyFailures = this.checkNgOnDestroy( sourceFile, componentClassDeclaration as ts.ClassDeclaration, - destroySubjectNamesUsed + Object.keys(destroySubjectNamesUsed) ); failures.push(...ngOnDestroyFailures); } @@ -148,7 +184,7 @@ export class Rule extends Lint.Rules.TypedRule { /** * Checks whether a .subscribe() is preceded by a .pipe(<...>, takeUntil(<...>)) */ - private ensureTakeuntilBeforeSubscribe( + private checkTakeuntilBeforeSubscribe( sourceFile: ts.SourceFile, node: ts.PropertyAccessExpression ): { failures: Lint.RuleFailure[]; destroySubjectName: string } { @@ -170,19 +206,16 @@ export class Rule extends Lint.Rules.TypedRule { if (pipedOperators.length > 0) { const lastPipedOperator = pipedOperators[pipedOperators.length - 1]; // check whether the last operator in the .pipe() call is takeUntil() - if ( - tsutils.isCallExpression(lastPipedOperator) && - tsutils.isIdentifier(lastPipedOperator.expression) && - lastPipedOperator.expression.text === "takeUntil" - ) { - lastTakeUntilFound = true; - // check the argument of takeUntil() - const destroySubjectNameCheck = this.checkDestroySubjectName( + if (tsutils.isCallExpression(lastPipedOperator)) { + const lastPipedOperatorFailures = this.checkTakeuntilOperator( sourceFile, lastPipedOperator ); - failures.push(...destroySubjectNameCheck.failures); - destroySubjectName = destroySubjectNameCheck.destroySubjectName; + if (lastPipedOperatorFailures.isTakeUntil) { + lastTakeUntilFound = true; + destroySubjectName = lastPipedOperatorFailures.destroySubjectName; + failures.push(...lastPipedOperatorFailures.failures); + } } } } @@ -203,6 +236,101 @@ export class Rule extends Lint.Rules.TypedRule { return { failures, destroySubjectName: destroySubjectName }; } + /** + * Checks whether there is a takeUntil() operator before operators like shareReplay() + */ + private checkTakeuntilBeforeOperatorsInPipe( + sourceFile: ts.SourceFile, + pipeArguments: ts.NodeArray + ): { failures: Lint.RuleFailure[]; destroySubjectNames: string[] } { + const failures: Lint.RuleFailure[] = []; + const destroySubjectNames: string[] = []; + + // go though all pipe arguments, i.e. rxjs operators + pipeArguments.forEach((pipeArgument, i) => { + // check whether the operator requires a preceding takeuntil + if ( + tsutils.isCallExpression(pipeArgument) && + tsutils.isIdentifier(pipeArgument.expression) && + this.operatorsRequiringPrecedingTakeuntil.includes( + pipeArgument.expression.getText() + ) + ) { + let precedingTakeUntilOperatorFound = false; + // check the preceding operator to be takeuntil + if ( + i > 0 && + pipeArguments[i - 1] && + tsutils.isCallExpression(pipeArguments[i - 1]) + ) { + const precedingOperator = pipeArguments[i - 1] as ts.CallExpression; + const precedingOperatorFailures = this.checkTakeuntilOperator( + sourceFile, + precedingOperator + ); + if (precedingOperatorFailures.isTakeUntil) { + precedingTakeUntilOperatorFound = true; + failures.push(...precedingOperatorFailures.failures); + if (precedingOperatorFailures.destroySubjectName) { + destroySubjectNames.push( + precedingOperatorFailures.destroySubjectName + ); + } + } + } + + if (!precedingTakeUntilOperatorFound) { + failures.push( + new Lint.RuleFailure( + sourceFile, + pipeArgument.getStart(), + pipeArgument.getStart() + pipeArgument.getWidth(), + Rule.FAILURE_STRING_OPERATOR.replace( + "{operator}", + pipeArgument.expression.getText() + ), + this.ruleName + ) + ); + } + } + }); + + return { failures, destroySubjectNames: destroySubjectNames }; + } + + /** + * Checks whether the operator given is takeUntil and uses an allowed destroy subject name + */ + private checkTakeuntilOperator( + sourceFile: ts.SourceFile, + operator: ts.CallExpression + ): { + failures: Lint.RuleFailure[]; + destroySubjectName: string; + isTakeUntil: boolean; + } { + const failures: Lint.RuleFailure[] = []; + let destroySubjectName: string; + let isTakeUntil: boolean = false; + + if ( + tsutils.isIdentifier(operator.expression) && + operator.expression.text === "takeUntil" + ) { + isTakeUntil = true; + // check the argument of takeUntil() + const destroySubjectNameCheck = this.checkDestroySubjectName( + sourceFile, + operator + ); + failures.push(...destroySubjectNameCheck.failures); + destroySubjectName = destroySubjectNameCheck.destroySubjectName; + } + + return { failures, destroySubjectName, isTakeUntil }; + } + /** * Checks whether the argument of the given takeUntil(this.destroy$) expression * is among the list of allowedDestroySubjectNames @@ -310,7 +438,7 @@ export class Rule extends Lint.Rules.TypedRule { } /** - * Checks whether all >destroySubjectNameUsed>.() are invoked in the ngOnDestroyMethod + * Checks whether all .() are invoked in the ngOnDestroyMethod */ private checkDestroySubjectMethodInvocation( sourceFile: ts.SourceFile, diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint index 6b199c9a..a0458119 100644 --- a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint @@ -1,5 +1,5 @@ import { combineLatest, of, Subject } from "rxjs"; -import { switchMap, takeUntil } from "rxjs/operators"; +import { switchMap, takeUntil, shareReplay, tap } from "rxjs/operators"; const a = of("a"); const b = of("b"); @@ -30,6 +30,9 @@ class MyComponent { private destroy$: Subject = new Subject(); + k$ = a.pipe(shareReplay(1)); + ~~~~~~~~~~~~~~ [enforce-takeuntil-before-operator-sharereplay] + someMethod() { const d = a.subscribe(); ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] @@ -39,11 +42,24 @@ class MyComponent { const f = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); - const g = a.pipe(takeUntil(d), s => switchMap(_ => b)).subscribe(); - ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + const g = a.pipe(takeUntil(this.destroy$), switchMap(_ => b)).subscribe(); + ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] - const f = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); + const h = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); ~ [enforce-takeuntil-before-unsubscribe-subject-name] + + const k1 = a.pipe(takeUntil(this.destroy$), shareReplay(1)).subscribe(); + ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + + const k = a.pipe(shareReplay(1), takeUntil(this.destroy$)).subscribe(); + ~~~~~~~~~~~~~~ [enforce-takeuntil-before-operator-sharereplay] + + const m = a.pipe(tap(), shareReplay(1), takeUntil(this.destroy$)).subscribe(); + ~~~~~~~~~~~~~~ [enforce-takeuntil-before-operator-sharereplay] + + const n = a.pipe(takeUntil(d), shareReplay(1), takeUntil(this.destroy$)).subscribe(); + ~ [enforce-takeuntil-before-unsubscribe-subject-name] + } } @@ -70,8 +86,12 @@ class MyComponent implements OnDestroy { class MyComponent implements SomeInterface, OnDestroy { private destroy$: Subject = new Subject(); + k$ = a.pipe(takeUntil(this.destroy$), shareReplay(1)); + someMethod() { const e = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); + + const k = a.pipe(takeUntil(this.destroy$), shareReplay(1), takeUntil(this.destroy$)).subscribe(); } ngOnDestroy() { @@ -86,3 +106,4 @@ class MyComponent implements SomeInterface, OnDestroy { [enforce-takeuntil-before-unsubscribe-ondestroy]: component containing subscribe must implement the ngOnDestroy() method [enforce-takeuntil-before-unsubscribe-next-missing]: there must be an invocation of this._destroy$.next() in ngOnDestroy() [enforce-takeuntil-before-unsubscribe-complete-missing]: there must be an invocation of this._destroy$.complete() in ngOnDestroy() +[enforce-takeuntil-before-operator-sharereplay]: the shareReplay operator used within a component must be preceded by takeUntil From 1eb042c244691b9330bbbf0ac8b1872f6c99b22a Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Fri, 8 Nov 2019 11:28:38 +0100 Subject: [PATCH 3/6] feat: improve rxjs-prefer-angular-takeuntil-before-subscribe rule rename to rxjs-prefer-angular-takeuntil-before-subscribe --- README.md | 23 +++++++++------ docs/index.md | 25 ++++++++++++----- ...ferAngularTakeuntilBeforeSubscribeRule.ts} | 2 +- .../default/fixture.ts.lint | 28 +++++++++---------- .../default/tsconfig.json | 0 .../default/tslint.json | 2 +- 6 files changed, 49 insertions(+), 31 deletions(-) rename source/rules/{rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts => rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts} (99%) rename test/v6/fixtures/{prefer-angular-takeuntil-before-unsubscribe => prefer-angular-takeuntil-before-subscribe}/default/fixture.ts.lint (74%) rename test/v6/fixtures/{prefer-angular-takeuntil-before-unsubscribe => prefer-angular-takeuntil-before-subscribe}/default/tsconfig.json (100%) rename test/v6/fixtures/{prefer-angular-takeuntil-before-unsubscribe => prefer-angular-takeuntil-before-subscribe}/default/tslint.json (59%) diff --git a/README.md b/README.md index 6da078b8..36bb21b6 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,7 @@ The package includes the following rules (none of which are enabled by default): | `rxjs-no-unsafe-takeuntil` | Disallows the application of operators after `takeUntil`. Operators placed after `takeUntil` can effect [subscription leaks](https://medium.com/@cartant/rxjs-avoiding-takeuntil-leaks-fb5182d047ef). | [See below](#rxjs-no-unsafe-takeuntil) | | `rxjs-no-unused-add` | Disallows the importation of patched observables or operators that are not used in the module. | None | | `rxjs-no-wholesale` | Disallows the wholesale importation of `rxjs` or `rxjs/Rx`. | None | -| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-angular-prefer-takeuntil-before-subscribe) | +| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-prefer-angular-takeuntil-before-subscribe) | | `rxjs-prefer-angular-async-pipe` | Disallows the calling of `subscribe` within an Angular component. | None | | `rxjs-prefer-observer` | Enforces the passing of observers to `subscribe` and `tap`. See [this RxJS issue](https://github.com/ReactiveX/rxjs/issues/4159). | [See below](#rxjs-prefer-observer) | | `rxjs-suffix-subjects` | Disalllows subjects that don't end with the specified `suffix` option. | [See below](#rxjs-suffix-subjects) | @@ -396,9 +396,9 @@ The following options are equivalent to the rule's default configuration: } ``` - + -#### rxjs-angular-prefer-takeuntil-before-subscribe +#### rxjs-prefer-angular-takeuntil-before-subscribe 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()` @@ -413,7 +413,7 @@ The following options are equivalent to the rule's default configuration: ```json "rules": { - "rxjs-angular-prefer-takeuntil-before-subscribe": { + "rxjs-prefer-angular-takeuntil-before-subscribe": { "options": [{ "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] }], @@ -424,23 +424,30 @@ The following options are equivalent to the rule's default configuration: ##### Example This should trigger an error: ```typescript - +@Component({ + selector: 'app-my', + template: '
{{k$ | async}}
' +}) class MyComponent { -// ~~~~~~~~~~~ component containing subscribe must implement the ngOnDestroy() method + ~~~~~~~~~~~ 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 + ~~~~~~~~~~~~~~ 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 + ~~~~~~~~~ subscribe within a component must be preceded by takeUntil } } ``` while this should be fine: ```typescript +@Component({ + selector: 'app-my', + template: '
{{k$ | async}}
' +}) class MyComponent implements SomeInterface, OnDestroy { private destroy$: Subject = new Subject(); diff --git a/docs/index.md b/docs/index.md index 028f47c4..10efb650 100644 --- a/docs/index.md +++ b/docs/index.md @@ -53,7 +53,7 @@ The package includes the following rules (none of which are enabled by default): | `rxjs-no-unsafe-takeuntil` | Disallows the application of operators after `takeUntil`. Operators placed after `takeUntil` can effect [subscription leaks](https://medium.com/@cartant/rxjs-avoiding-takeuntil-leaks-fb5182d047ef). | [See below](#rxjs-no-unsafe-takeuntil) | | `rxjs-no-unused-add` | Disallows the importation of patched observables or operators that are not used in the module. | None | | `rxjs-no-wholesale` | Disallows the wholesale importation of `rxjs` or `rxjs/Rx`. | None | -| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-angular-prefer-takeuntil-before-subscribe) | +| `rxjs-prefer-angular-takeuntil-before-subscribe` | Enforces the application of the `takeUntil` operator when calling of `subscribe` within an Angular component. | [See below](#rxjs-prefer-angular-takeuntil-before-subscribe) | | `rxjs-prefer-angular-async-pipe` | Disallows the calling of `subscribe` within an Angular component. | None | | `rxjs-prefer-observer` | Enforces the passing of observers to `subscribe` and `tap`. See [this RxJS issue](https://github.com/ReactiveX/rxjs/issues/4159). | [See below](#rxjs-prefer-observer) | | `rxjs-suffix-subjects` | Disalllows subjects that don't end with the specified `suffix` option. | [See below](#rxjs-suffix-subjects) | @@ -334,7 +334,10 @@ The following options are equivalent to the rule's default configuration: -#### rxjs-angular-prefer-takeuntil-before-subscribe + + + +#### rxjs-prefer-angular-takeuntil-before-subscribe 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()` @@ -349,7 +352,7 @@ The following options are equivalent to the rule's default configuration: ```json "rules": { - "rxjs-angular-prefer-takeuntil-before-subscribe": { + "rxjs-prefer-angular-takeuntil-before-subscribe": { "options": [{ "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] }], @@ -360,23 +363,30 @@ The following options are equivalent to the rule's default configuration: ##### Example This should trigger an error: ```typescript - +@Component({ + selector: 'app-my', + template: '
{{k$ | async}}
' +}) class MyComponent { -// ~~~~~~~~~~~ component containing subscribe must implement the ngOnDestroy() method + ~~~~~~~~~~~ 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 + ~~~~~~~~~~~~~~ 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 + ~~~~~~~~~ subscribe within a component must be preceded by takeUntil } } ``` while this should be fine: ```typescript +@Component({ + selector: 'app-my', + template: '
{{k$ | async}}
' +}) class MyComponent implements SomeInterface, OnDestroy { private destroy$: Subject = new Subject(); @@ -393,6 +403,7 @@ class MyComponent implements SomeInterface, OnDestroy { } ``` + #### rxjs-prefer-observer diff --git a/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts similarity index 99% rename from source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts rename to source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts index 830beaa4..4ee160ad 100644 --- a/source/rules/rxjsPreferAngularTakeuntilBeforeUnsubscribeRule.ts +++ b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts @@ -25,7 +25,7 @@ export class Rule extends Lint.Rules.TypedRule { An optional object with optional \`destroySubjectNames\` property. The property is an array containing the allowed subject names expected as argument of \`takeUntil\`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$'].`, requiresTypeInfo: true, - ruleName: "rxjs-prefer-angular-takeuntil-before-unsubscribe", + ruleName: "rxjs-prefer-angular-takeuntil-before-subscribe", type: "functionality", typescriptOnly: true }; diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint similarity index 74% rename from test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint rename to test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint index a0458119..6025e0ca 100644 --- a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/fixture.ts.lint +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint @@ -26,7 +26,7 @@ class MyClass { selector: 'app-my' }) class MyComponent { - ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-ondestroy] + ~~~~~~~~~~~ [enforce-takeuntil-before-subscribe-ondestroy] private destroy$: Subject = new Subject(); @@ -35,21 +35,21 @@ class MyComponent { someMethod() { const d = a.subscribe(); - ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + ~~~~~~~~~ [enforce-takeuntil-before-subscribe] const e = a.pipe(switchMap(_ => b)).subscribe(); - ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + ~~~~~~~~~ [enforce-takeuntil-before-subscribe] const f = a.pipe(switchMap(_ => b), takeUntil(this.destroy$)).subscribe(); const g = a.pipe(takeUntil(this.destroy$), switchMap(_ => b)).subscribe(); - ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + ~~~~~~~~~ [enforce-takeuntil-before-subscribe] const h = a.pipe(switchMap(_ => b), takeUntil(d)).subscribe(); - ~ [enforce-takeuntil-before-unsubscribe-subject-name] + ~ [enforce-takeuntil-before-subscribe-subject-name] const k1 = a.pipe(takeUntil(this.destroy$), shareReplay(1)).subscribe(); - ~~~~~~~~~ [enforce-takeuntil-before-unsubscribe] + ~~~~~~~~~ [enforce-takeuntil-before-subscribe] const k = a.pipe(shareReplay(1), takeUntil(this.destroy$)).subscribe(); ~~~~~~~~~~~~~~ [enforce-takeuntil-before-operator-sharereplay] @@ -58,7 +58,7 @@ class MyComponent { ~~~~~~~~~~~~~~ [enforce-takeuntil-before-operator-sharereplay] const n = a.pipe(takeUntil(d), shareReplay(1), takeUntil(this.destroy$)).subscribe(); - ~ [enforce-takeuntil-before-unsubscribe-subject-name] + ~ [enforce-takeuntil-before-subscribe-subject-name] } } @@ -72,8 +72,8 @@ class MyComponent implements OnDestroy { } ngOnDestroy() { - ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-next-missing] - ~~~~~~~~~~~ [enforce-takeuntil-before-unsubscribe-complete-missing] + ~~~~~~~~~~~ [enforce-takeuntil-before-subscribe-next-missing] + ~~~~~~~~~~~ [enforce-takeuntil-before-subscribe-complete-missing] // this._destroy$.next() is missing this.destroy$.next(); this.destroy$.complete(); @@ -101,9 +101,9 @@ class MyComponent implements SomeInterface, OnDestroy { } -[enforce-takeuntil-before-unsubscribe]: subscribe within a component must be preceded by takeUntil -[enforce-takeuntil-before-unsubscribe-subject-name]: takeUntil argument must be one of [this.destroy$, this._destroy$] -[enforce-takeuntil-before-unsubscribe-ondestroy]: component containing subscribe must implement the ngOnDestroy() method -[enforce-takeuntil-before-unsubscribe-next-missing]: there must be an invocation of this._destroy$.next() in ngOnDestroy() -[enforce-takeuntil-before-unsubscribe-complete-missing]: there must be an invocation of this._destroy$.complete() in ngOnDestroy() +[enforce-takeuntil-before-subscribe]: subscribe within a component must be preceded by takeUntil +[enforce-takeuntil-before-subscribe-subject-name]: takeUntil argument must be one of [this.destroy$, this._destroy$] +[enforce-takeuntil-before-subscribe-ondestroy]: component containing subscribe must implement the ngOnDestroy() method +[enforce-takeuntil-before-subscribe-next-missing]: there must be an invocation of this._destroy$.next() in ngOnDestroy() +[enforce-takeuntil-before-subscribe-complete-missing]: there must be an invocation of this._destroy$.complete() in ngOnDestroy() [enforce-takeuntil-before-operator-sharereplay]: the shareReplay operator used within a component must be preceded by takeUntil diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/tsconfig.json similarity index 100% rename from test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tsconfig.json rename to test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/tsconfig.json diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/tslint.json similarity index 59% rename from test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json rename to test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/tslint.json index 3b91e898..a2160ab8 100644 --- a/test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/default/tslint.json +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/tslint.json @@ -2,7 +2,7 @@ "defaultSeverity": "error", "jsRules": {}, "rules": { - "rxjs-prefer-angular-takeuntil-before-unsubscribe": { "severity": "error" } + "rxjs-prefer-angular-takeuntil-before-subscribe": { "severity": "error" } }, "rulesDirectory": "../../../../../build/rules" } From 031b5e99379a2e40bf18b06bf3c0034523f6609b Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Fri, 8 Nov 2019 14:42:05 +0100 Subject: [PATCH 4/6] feat: improve rxjs-prefer-angular-takeuntil-before-subscribe rule fix NPE for complex class --- .../rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts index 4ee160ad..674f5f48 100644 --- a/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts +++ b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts @@ -108,8 +108,6 @@ export class Rule extends Lint.Rules.TypedRule { const failures: Lint.RuleFailure[] = []; const typeChecker = program.getTypeChecker(); - /** whether there is at least one observable.subscribe() expression */ - let hasSubscribeInComponent = false; /** list of destroy subjects used in takeUntil() operators */ const destroySubjectNamesUsed: { [destroySubjectName: string]: boolean; @@ -136,7 +134,6 @@ export class Rule extends Lint.Rules.TypedRule { if (subscribeFailures.destroySubjectName) { destroySubjectNamesUsed[subscribeFailures.destroySubjectName] = true; } - hasSubscribeInComponent = true; } }); @@ -164,16 +161,16 @@ export class Rule extends Lint.Rules.TypedRule { destroySubjectNamesUsed[destroySubjectName] = true; } }); - hasSubscribeInComponent = true; } }); // check the ngOnDestroyMethod - if (hasSubscribeInComponent) { + const destroySubjectNamesUsedList = Object.keys(destroySubjectNamesUsed); + if (destroySubjectNamesUsedList.length > 0) { const ngOnDestroyFailures = this.checkNgOnDestroy( sourceFile, componentClassDeclaration as ts.ClassDeclaration, - Object.keys(destroySubjectNamesUsed) + destroySubjectNamesUsedList ); failures.push(...ngOnDestroyFailures); } @@ -401,7 +398,7 @@ export class Rule extends Lint.Rules.TypedRule { ): Lint.RuleFailure[] { const failures: Lint.RuleFailure[] = []; const ngOnDestroyMethod = classDeclaration.members.find( - member => member.name.getText() === "ngOnDestroy" + member => member.name && member.name.getText() === "ngOnDestroy" ); // check whether the ngOnDestroy method is implemented From 194602262ac2d6b3c47edc9686c6f2c087a1521d Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Tue, 12 Nov 2019 07:58:47 +0100 Subject: [PATCH 5/6] feat: improve rxjs-prefer-angular-takeuntil-before-subscribe rule remove configuration options --- README.md | 17 +------ docs/index.md | 18 +------ ...eferAngularTakeuntilBeforeSubscribeRule.ts | 51 +++---------------- .../default/fixture.ts.lint | 2 +- 4 files changed, 9 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index 36bb21b6..9e75173e 100644 --- a/README.md +++ b/README.md @@ -401,26 +401,11 @@ The following options are equivalent to the rule's default configuration: #### rxjs-prefer-angular-takeuntil-before-subscribe 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()` +by enforcing the application of the `takeUntil(this.destroy$)` 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()`. -The rule takes an optional object with an optional `allowedDestroySubjectNames` property, -which is an array containing the allowed subject names expected as argument of `takeUntil`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$']. - -The following options are equivalent to the rule's default configuration: - -```json -"rules": { - "rxjs-prefer-angular-takeuntil-before-subscribe": { - "options": [{ - "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] - }], - "severity": "error" - } -} -``` ##### Example This should trigger an error: ```typescript diff --git a/docs/index.md b/docs/index.md index 10efb650..4b0c31a1 100644 --- a/docs/index.md +++ b/docs/index.md @@ -340,26 +340,11 @@ The following options are equivalent to the rule's default configuration: #### rxjs-prefer-angular-takeuntil-before-subscribe 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()` +by enforcing the application of the `takeUntil(this.destroy$)` 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()`. -The rule takes an optional object with an optional `allowedDestroySubjectNames` property, -which is an array containing the allowed subject names expected as argument of `takeUntil`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$']. - -The following options are equivalent to the rule's default configuration: - -```json -"rules": { - "rxjs-prefer-angular-takeuntil-before-subscribe": { - "options": [{ - "allowedDestroySubjectNames": ["destroy$", "_destroy$", "destroyed$", "_destroyed$"] - }], - "severity": "error" - } -} -``` ##### Example This should trigger an error: ```typescript @@ -403,7 +388,6 @@ class MyComponent implements SomeInterface, OnDestroy { } ``` - #### rxjs-prefer-observer diff --git a/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts index 674f5f48..19c90641 100644 --- a/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts +++ b/source/rules/rxjsPreferAngularTakeuntilBeforeSubscribeRule.ts @@ -15,15 +15,8 @@ export class Rule extends Lint.Rules.TypedRule { public static metadata: Lint.IRuleMetadata = { description: dedent`Enforces the application of the takeUntil operator when calling of subscribe within an Angular component.`, - options: { - properties: { - allowedDestroySubjectNames: { type: "array", items: { type: "string" } } - }, - type: "object" - }, - optionsDescription: Lint.Utils.dedent` - An optional object with optional \`destroySubjectNames\` property. - The property is an array containing the allowed subject names expected as argument of \`takeUntil\`, defaults to ['destroy$', '_destroy$', 'destroyed$', '_destroyed$'].`, + options: null, + optionsDescription: "", requiresTypeInfo: true, ruleName: "rxjs-prefer-angular-takeuntil-before-subscribe", type: "functionality", @@ -34,7 +27,7 @@ export class Rule extends Lint.Rules.TypedRule { "subscribe within a component must be preceded by takeUntil"; public static FAILURE_STRING_SUBJECT_NAME = - "takeUntil argument must be one of {allowedDestroySubjectNames}"; + "takeUntil argument must be a property of the class, e.g. takeUntil(this.destroy$)"; public static FAILURE_STRING_OPERATOR = "the {operator} operator used within a component must be preceded by takeUntil"; @@ -45,8 +38,6 @@ export class Rule extends Lint.Rules.TypedRule { public static FAILURE_STRING_NG_ON_DESTROY_SUBJECT_METHOD_NOT_CALLED = "there must be an invocation of {destroySubjectName}.{methodName}() in ngOnDestroy()"; - private allowedDestroySubjectNames: string[] = ["destroy$", "_destroy$"]; - private operatorsRequiringPrecedingTakeuntil: string[] = [ "publish", "publishBehavior", @@ -61,24 +52,6 @@ export class Rule extends Lint.Rules.TypedRule { ): Lint.RuleFailure[] { const failures: Lint.RuleFailure[] = []; - // initialize the options - const options = this.getOptions(); - if (options.ruleArguments) { - const allowedDestroySubjectNamesOption = options.ruleArguments.find( - ruleArgument => - ruleArgument.hasOwnProperty("allowedDestroySubjectNames") - ); - if ( - allowedDestroySubjectNamesOption && - Array.isArray( - allowedDestroySubjectNamesOption["allowedDestroySubjectNames"] - ) - ) { - this.allowedDestroySubjectNames = - allowedDestroySubjectNamesOption["allowedDestroySubjectNames"]; - } - } - // find all classes with an @Component() decorator const componentClassDeclarations = tsquery( sourceFile, @@ -330,7 +303,7 @@ export class Rule extends Lint.Rules.TypedRule { /** * Checks whether the argument of the given takeUntil(this.destroy$) expression - * is among the list of allowedDestroySubjectNames + * is a property of the class */ private checkDestroySubjectName( sourceFile: ts.SourceFile, @@ -357,12 +330,7 @@ export class Rule extends Lint.Rules.TypedRule { takeUntilOperatorArgument = takeUntilOperator .arguments[0] as ts.PropertyAccessExpression; destroySubjectName = takeUntilOperatorArgument.name.getText(); - - isAllowedDestroySubject = this.allowedDestroySubjectNames.some( - allowedDestroySubjectName => - takeUntilOperatorArgument.name.getText() === - allowedDestroySubjectName - ); + isAllowedDestroySubject = true; } } @@ -372,14 +340,7 @@ export class Rule extends Lint.Rules.TypedRule { sourceFile, highlightedNode.getStart(), highlightedNode.getStart() + highlightedNode.getWidth(), - Rule.FAILURE_STRING_SUBJECT_NAME.replace( - "{allowedDestroySubjectNames}", - "[" + - this.allowedDestroySubjectNames - .map(name => "this." + name) - .join(", ") + - "]" - ), + Rule.FAILURE_STRING_SUBJECT_NAME, this.ruleName ) ); diff --git a/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint index 6025e0ca..e0a16198 100644 --- a/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint +++ b/test/v6/fixtures/prefer-angular-takeuntil-before-subscribe/default/fixture.ts.lint @@ -102,7 +102,7 @@ class MyComponent implements SomeInterface, OnDestroy { [enforce-takeuntil-before-subscribe]: subscribe within a component must be preceded by takeUntil -[enforce-takeuntil-before-subscribe-subject-name]: takeUntil argument must be one of [this.destroy$, this._destroy$] +[enforce-takeuntil-before-subscribe-subject-name]: takeUntil argument must be a property of the class, e.g. takeUntil(this.destroy$) [enforce-takeuntil-before-subscribe-ondestroy]: component containing subscribe must implement the ngOnDestroy() method [enforce-takeuntil-before-subscribe-next-missing]: there must be an invocation of this._destroy$.next() in ngOnDestroy() [enforce-takeuntil-before-subscribe-complete-missing]: there must be an invocation of this._destroy$.complete() in ngOnDestroy() From 65326ea2eb0a944bb8f99eb299904af6e704d3f9 Mon Sep 17 00:00:00 2001 From: Esteban Gehring Date: Tue, 12 Nov 2019 08:16:19 +0100 Subject: [PATCH 6/6] feat: improve rxjs-prefer-angular-takeuntil-before-subscribe rule code cleanup --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 5432c9df..8e1273b6 100644 --- a/package.json +++ b/package.json @@ -83,7 +83,6 @@ "test:mocha": "mocha \"./build/**/*-spec.js\"", "test:tslint-v5": "yarn --cwd ./test/v5 install && yarn --cwd ./test/v5 upgrade && tslint --test \"./test/v5/fixtures/**/tslint.json\"", "test:tslint-v6": "yarn --cwd ./test/v6 install && yarn --cwd ./test/v6 upgrade && tslint --test \"./test/v6/fixtures/**/tslint.json\"", - "special-takeuntil": "npm run test:build && tslint --test \"test/v6/fixtures/prefer-angular-takeuntil-before-unsubscribe/**/tslint.json\"", "test:tslint-v6-compat": "yarn --cwd ./test/v6-compat install && yarn --cwd ./test/v6-compat upgrade && tslint --test \"./test/v6-compat/fixtures/**/tslint.json\"" }, "version": "4.26.1"