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

feat: Add option to check takeUntil within all angular entities #116

Merged
merged 2 commits into from
May 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ The package includes the following rules (none of which are enabled by default):
| `rxjs-no-wholesale` | Disallows the wholesale importation of `rxjs` or `rxjs/Rx`. | None |
| `rxjs-prefer-angular-async-pipe` | Disallows the calling of `subscribe` within an Angular component. | None |
| `rxjs-prefer-angular-composition` | Enforces the composition of subscriptions within an Angular component. The rule ensures that subscriptions are composed into a class-property `Subscription` and that the `Subscription` is unsubscribed in `ngOnDestroy`. (For an example, see [the tests](https://github.com/cartant/rxjs-tslint-rules/blob/0f53f9258bfe573c9e8948b1381bd446664cf5f9/test/v6/fixtures/prefer-angular-composition/default/fixture.ts.lint#L4-L17).) | None |
| `rxjs-prefer-angular-takeuntil` | Enforces the application of the `takeUntil` operator when calling `subscribe` within an Angular component. The rule (optionally) ensures that `takeUntil` is passed a class-property `Subject` and that the `Subject` is notified in `ngOnDestroy`. (For an example, see [the tests](https://github.com/cartant/rxjs-tslint-rules/blob/0f53f9258bfe573c9e8948b1381bd446664cf5f9/test/v6/fixtures/prefer-angular-takeuntil/default/fixture.ts.lint#L10-L25).) | [See below](#rxjs-prefer-angular-takeuntil) |
| `rxjs-prefer-angular-takeuntil` | Enforces the application of the `takeUntil` operator when calling `subscribe` within an Angular component (optionally within classes with one of specified decorators). The rule (optionally) ensures that `takeUntil` is passed a class-property `Subject` and that the `Subject` is notified in `ngOnDestroy`. (For an example, see [the tests](https://github.com/cartant/rxjs-tslint-rules/blob/0f53f9258bfe573c9e8948b1381bd446664cf5f9/test/v6/fixtures/prefer-angular-takeuntil/default/fixture.ts.lint#L10-L25).) | [See below](#rxjs-prefer-angular-takeuntil) |
| `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) |
| `rxjs-throw-error` | Enforces the passing of `Error` values to `error` notifications. | None |
Expand Down Expand Up @@ -342,7 +342,7 @@ The following options are equivalent to the rule's default configuration:

#### rxjs-prefer-angular-takeuntil

The rule takes an optional object with optional `alias` and `checkDestroy` properties. The `alias` property is an array containing the names of operators that aliases for `takeUntil`. And the `checkDestroy` property is a boolean that determines whether or not a `Subject`-based `ngOnDestroy` must be implemented.
The rule takes an optional object with optional `alias` and `checkDestroy` properties. The `alias` property is an array containing the names of operators that aliases for `takeUntil`. The `checkDecorators` property is an array containing the names of decorators, when using which it is necessary to check the class. And the `checkDestroy` property is a boolean that determines whether or not a `Subject`-based `ngOnDestroy` must be implemented.

<a name="rxjs-prefer-observer"></a>

Expand Down
39 changes: 23 additions & 16 deletions source/rules/rxjsPreferAngularTakeuntilRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,27 @@ import { couldBeType, isThis } from "../support/util";
type Options = {
alias: string[];
checkDestroy: boolean;
checkDecorators: string[];
};

export class Rule extends Lint.Rules.TypedRule {
public static metadata: Lint.IRuleMetadata = {
deprecationMessage: peer.v5 ? peer.v5NotSupportedMessage : undefined,
description: Lint.Utils
.dedent`Enforces the application of the takeUntil operator
when calling subscribe within an Angular component.`,
when calling subscribe within an Angular component (optionally within services, directives, pipes).`,
options: {
properties: {
alias: { type: "array", items: { type: "string" } },
checkDecorators: { type: "array", items: { type: "string" } },
checkDestroy: { type: "boolean" }
},
type: "object"
},
optionsDescription: Lint.Utils.dedent`
An optional object with optional \`alias\` and \`checkDestroy\` properties.
An optional object with optional \`alias\`, \`checkDecorators\` and \`checkDestroy\` properties.
The \`alias\` property is an array containing the names of operators that aliases for \`takeUntil\`.
The \`checkDecorators\` property is an array containing the names of decorators, when using which it is necessary to check the class.
The \`checkDestroy\` property is a boolean that determines whether or not a \`Subject\`-based \`ngOnDestroy\` must be implemented.`,
requiresTypeInfo: true,
ruleName: "rxjs-prefer-angular-takeuntil",
Expand Down Expand Up @@ -63,21 +66,25 @@ export class Rule extends Lint.Rules.TypedRule {
// If an alias is specified, check for the subject-based destroy only if
// it's explicitly configured. It's extremely unlikely a subject-based
// destroy mechanism will be used in conjunction with an alias.
const { alias = [], checkDestroy = alias.length === 0 }: Options =
options || {};
const {
alias = [],
checkDestroy = alias.length === 0,
checkDecorators = ["Component"]
}: Options = options || {};

// find all classes with a @Component() decorator
const componentClassDeclarations = tsquery(
// find all classes with given decorators
const decoratorQuery = `/^(${checkDecorators.join("|")})$/`;
const classDeclarations = tsquery(
sourceFile,
`ClassDeclaration:has(Decorator[expression.expression.name='Component'])`
`ClassDeclaration:has(Decorator[expression.expression.name=${decoratorQuery}])`
) as ts.ClassDeclaration[];
componentClassDeclarations.forEach(componentClassDeclaration => {
classDeclarations.forEach(classDeclaration => {
failures.push(
...this.checkComponentClassDeclaration(
...this.checkClassDeclaration(
sourceFile,
program,
{ alias, checkDestroy },
componentClassDeclaration
{ alias, checkDestroy, checkDecorators },
classDeclaration
)
);
});
Expand All @@ -86,13 +93,13 @@ export class Rule extends Lint.Rules.TypedRule {
}

/**
* Checks a component class for occurrences of .subscribe() and corresponding takeUntil() requirements
* Checks a class for occurrences of .subscribe() and corresponding takeUntil() requirements
*/
private checkComponentClassDeclaration(
private checkClassDeclaration(
sourceFile: ts.SourceFile,
program: ts.Program,
options: Options,
componentClassDeclaration: ts.ClassDeclaration
classDeclaration: ts.ClassDeclaration
): Lint.RuleFailure[] {
const failures: Lint.RuleFailure[] = [];
const typeChecker = program.getTypeChecker();
Expand All @@ -103,7 +110,7 @@ export class Rule extends Lint.Rules.TypedRule {

// find observable.subscribe() call expressions
const subscribePropertyAccessExpressions = tsquery(
componentClassDeclaration,
classDeclaration,
`CallExpression > PropertyAccessExpression[name.name="subscribe"]`
) as ts.PropertyAccessExpression[];

Expand Down Expand Up @@ -141,7 +148,7 @@ export class Rule extends Lint.Rules.TypedRule {
failures.push(
...this.checkNgOnDestroy(
sourceFile,
componentClassDeclaration,
classDeclaration,
destroySubjectNamesBySubscribes
)
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { Component, OnDestroy } from "@angular/core";
import { 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");

// COMPONENTS

@Component({
selector: "correct-component"
})
class CorrectComponent implements OnDestroy {
private destroy = new Subject<void>();
someMethod() {
a.pipe(
switchMap(_ => b),
takeUntil(this.destroy)
).subscribe();
}
ngOnDestroy() {
this.destroy.next();
this.destroy.complete();
}
}

@Component({
selector: "no-next-component"
})
class NoTakeUntilComponent {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
~~~~~~~~~ [prefer-takeuntil]
}
}

// SERVICES

@Injectable()
class CorrectService implements OnDestroy {
private destroy = new Subject<void>();
someMethod() {
a.pipe(
switchMap(_ => b)
takeUntil(this.destroy)
).subscribe();
}
ngOnDestroy() {
this.destroy.next();
this.destroy.complete();
}
}

@Injectable()
class NoTakeUntilService {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
~~~~~~~~~ [prefer-takeuntil]
}
}

// PIPES

@Pipe({
name: 'controlByName',
})
class CorrectPipe implements OnDestroy {
private destroy = new Subject<void>();
someMethod() {
a.pipe(
switchMap(_ => b)
takeUntil(this.destroy)
).subscribe();
}
ngOnDestroy() {
this.destroy.next();
this.destroy.complete();
}
}

@Pipe({
name: 'controlByName',
})
class NoTakeUntilPipe {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
~~~~~~~~~ [prefer-takeuntil]
}
}

// DIRECTIVES

@Directive({
selector: 'my-directive'
})
class CorrectDirective implements OnDestroy {
private destroy = new Subject<void>();
someMethod() {
a.pipe(
switchMap(_ => b)
takeUntil(this.destroy)
).subscribe();
}
ngOnDestroy() {
this.destroy.next();
this.destroy.complete();
}
}


@Directive({
selector: 'my-directive'
})
class NoTakeUntilDirective {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
~~~~~~~~~ [prefer-takeuntil]
}
}

[prefer-takeuntil]: Subscribing without takeUntil is forbidden
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"compilerOptions": {
"baseUrl": ".",
"lib": ["es2015"],
"noEmit": true,
"paths": {
"rxjs": ["../../node_modules/rxjs"]
},
"skipLibCheck": true,
"target": "es5"
},
"include": ["fixture.ts"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"defaultSeverity": "error",
"jsRules": {},
"rules": {
"rxjs-prefer-angular-takeuntil": {
"options": [{
"checkDecorators": [
"Component",
"Pipe",
"Injectable",
"Directive"
]
}],
"severity": "error"
}
},
"rulesDirectory": "../../../../../build/rules"
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,34 @@ class SomeFunction {
a.pipe(switchMap(_ => b)).subscribe();
a.pipe(switchMap(_ => b), takeUntil(d)).subscribe();
}

@Injectable()
class NoTakeUntilService {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
}
}

@Pipe({
name: 'controlByName',
})
class NoTakeUntilPipe {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
}
}

@Directive({
selector: 'my-directive'
})
class NoTakeUntilDirective {
someMethod() {
a.pipe(
switchMap(_ => b)
).subscribe();
}
}