diff --git a/README.md b/README.md index 4f1ebc45..b83b755e 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,7 @@ The package includes the following rules: | `rxjs-no-subject-unsubscribe` | Disallows the calling of `unsubscribe` directly upon `Subject` instances. For an explanation of why this can be a problem, see [this](https://stackoverflow.com/a/45112125/6680611) Stack Overflow answer. | None | | `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-no-ignored-error` | Disallows errors ignoring in Observable's .subscribe() calls. | None | ### Options diff --git a/docs/index.md b/docs/index.md index f85fb576..61a8e721 100644 --- a/docs/index.md +++ b/docs/index.md @@ -13,6 +13,7 @@ The package includes the following rules: | `rxjs-no-subject-unsubscribe` | Disallows the calling of `unsubscribe` directly upon `Subject` instances. For an explanation of why this can be a problem, see [this](https://stackoverflow.com/a/45112125/6680611) Stack Overflow answer. | None | | `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-no-ignored-error` | Disallows errors ignoring in Observable's .subscribe() calls. | None | ### Options diff --git a/fixtures/subscription-with-error-handler/fixture.ts b/fixtures/subscription-with-error-handler/fixture.ts new file mode 100644 index 00000000..92cb17f3 --- /dev/null +++ b/fixtures/subscription-with-error-handler/fixture.ts @@ -0,0 +1,11 @@ +import { Subject } from "rxjs/Subject"; +import { of } from "rxjs/Observable/of"; + +const subject = new Subject(); +const observable = of([1, 2]); + +observable.subscribe(function () { }, () => 0); + +// When first parameter is not a function, should not trigger an error +observable.subscribe(subject); + diff --git a/fixtures/subscription-with-error-handler/tsconfig.json b/fixtures/subscription-with-error-handler/tsconfig.json new file mode 100644 index 00000000..09a2e6e7 --- /dev/null +++ b/fixtures/subscription-with-error-handler/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/fixtures/subscription-with-error-handler/tslint.json b/fixtures/subscription-with-error-handler/tslint.json new file mode 100644 index 00000000..dd04f8ae --- /dev/null +++ b/fixtures/subscription-with-error-handler/tslint.json @@ -0,0 +1,10 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-no-ignored-error": { + "severity": "error" + } + }, + "rulesDirectory": "../../build/rules" +} diff --git a/fixtures/subscription-without-error-handler/fixture-observable.ts b/fixtures/subscription-without-error-handler/fixture-observable.ts new file mode 100644 index 00000000..f02f25a7 --- /dev/null +++ b/fixtures/subscription-without-error-handler/fixture-observable.ts @@ -0,0 +1,6 @@ +import { Observable } from "rxjs/Observable"; +import "rxjs/add/observable/of"; + +const observable = Observable.of([1, 2]); +observable.subscribe(() => 0); + diff --git a/fixtures/subscription-without-error-handler/fixture-subject.ts b/fixtures/subscription-without-error-handler/fixture-subject.ts new file mode 100644 index 00000000..bc74aa82 --- /dev/null +++ b/fixtures/subscription-without-error-handler/fixture-subject.ts @@ -0,0 +1,5 @@ +import { Subject } from "rxjs/Subject"; + +const subject = new Subject(); +subject.subscribe(() => 0); + diff --git a/fixtures/subscription-without-error-handler/fixture-with-parameter.ts b/fixtures/subscription-without-error-handler/fixture-with-parameter.ts new file mode 100644 index 00000000..47bc8a36 --- /dev/null +++ b/fixtures/subscription-without-error-handler/fixture-with-parameter.ts @@ -0,0 +1,11 @@ +import { Subject } from "rxjs/Subject"; +import { Observable } from "rxjs/Observable"; +import "rxjs/add/observable/of"; + +const observable = Observable.of(1, 2); +const subject = new Subject(); +const handler = () => 0; +// Should mark it as error +observable.subscribe(handler); +// Should be ok - first parameter is not a function +observable.subscribe(subject); diff --git a/fixtures/subscription-without-error-handler/tsconfig.json b/fixtures/subscription-without-error-handler/tsconfig.json new file mode 100644 index 00000000..2f536568 --- /dev/null +++ b/fixtures/subscription-without-error-handler/tsconfig.json @@ -0,0 +1,13 @@ +{ + "compilerOptions": { + "baseUrl": ".", + "lib": ["es2015"], + "noEmit": true, + "paths": { + "rxjs": ["../node_modules/rxjs"] + }, + "skipLibCheck": true, + "target": "es5" + }, + "include": ["fixture-observable.ts", "fixture-subject.ts", "fixture-with-parameter.ts"] +} diff --git a/fixtures/subscription-without-error-handler/tslint.json b/fixtures/subscription-without-error-handler/tslint.json new file mode 100644 index 00000000..dd04f8ae --- /dev/null +++ b/fixtures/subscription-without-error-handler/tslint.json @@ -0,0 +1,10 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-no-ignored-error": { + "severity": "error" + } + }, + "rulesDirectory": "../../build/rules" +} diff --git a/source/fixtures-spec.ts b/source/fixtures-spec.ts index 904c91df..393cb283 100644 --- a/source/fixtures-spec.ts +++ b/source/fixtures-spec.ts @@ -471,20 +471,50 @@ describe("fixtures", function (): void { }); }); + describe("subscription related rules", () => { + describe("subscription-without-error-handler", () => { + it("should effect an 'rxjs-no-ignored-error' error on Observable", () => { + const result = lint("subscription-without-error-handler", "tslint.json", "fixture-observable.ts"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-ignored-error"); + }); + }); + describe("subscription-without-error-handler", () => { + it("should effect an 'rxjs-no-ignored-error' error on Observable's ancestors", () => { + const result = lint("subscription-without-error-handler", "tslint.json", "fixture-subject.ts"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-ignored-error"); + }); + }); + describe("subscription-without-error-handler", () => { + it("should effect an 'rxjs-no-ignored-error' error even if first parameter is a funciton variable", () => { + const result = lint("subscription-without-error-handler", "tslint.json", "fixture-with-parameter.ts"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-ignored-error"); + }); + }); + describe("subscription-with-error-handler", () => { + it("should not produce errors", () => { + const result = lint("subscription-with-error-handler", "tslint.json"); + expect(result).to.have.property("errorCount", 0); + }); + }); + }); + function lint(dir: string, configFileName?: string, fixtureFileName?: string): LintResult { - const fileName = `./fixtures/${dir}/${fixtureFileName || "fixture.ts"}`; - const content = fs.readFileSync(fileName, "utf8"); - const program = Linter.createProgram(`./fixtures/${dir}/tsconfig.json`); - const linter = new Linter({ fix: false }, program); - - const configuration = Configuration.findConfiguration( - configFileName ? - `./fixtures/${dir}/${configFileName}` : - `./fixtures/tslint.json`, - fileName - ).results; - linter.lint(fileName, content, configuration); - return linter.getResult(); + const fileName = `./fixtures/${dir}/${fixtureFileName || "fixture.ts"}`; + const content = fs.readFileSync(fileName, "utf8"); + const program = Linter.createProgram(`./fixtures/${dir}/tsconfig.json`); + const linter = new Linter({ fix: false }, program); + + const configuration = Configuration.findConfiguration( + configFileName ? + `./fixtures/${dir}/${configFileName}` : + `./fixtures/tslint.json`, + fileName + ).results; + linter.lint(fileName, content, configuration); + return linter.getResult(); } }); diff --git a/source/rules/rxjsNoIgnoredErrorRule.ts b/source/rules/rxjsNoIgnoredErrorRule.ts new file mode 100644 index 00000000..94a5b295 --- /dev/null +++ b/source/rules/rxjsNoIgnoredErrorRule.ts @@ -0,0 +1,74 @@ +/** + * @license Copyright © 2018 Alexey Petushkov. All Rights Reserved. + * 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 ts from "typescript"; + +import { couldBeType, isReferenceType } from "../support/util"; +import { InternalSymbolName } from "typescript"; + +export class Rule extends Lint.Rules.TypedRule { + + public static metadata: Lint.IRuleMetadata = { + description: "Disallows the calling of subscribe without specifying a error handler.", + options: null, + optionsDescription: "Not configurable.", + requiresTypeInfo: true, + ruleName: "rxjs-no-ignored-error", + type: "functionality", + typescriptOnly: true + }; + + public static FAILURE_STRING = "Calling subscribe without a second parameter is forbidden"; + + public applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] { + + return this.applyWithWalker(new Walker(sourceFile, this.getOptions(), program)); + } +} + +export class Walker extends Lint.ProgramAwareRuleWalker { + + protected visitCallExpression(node: ts.CallExpression): void { + + node.forEachChild((child) => { + + if (child.kind === ts.SyntaxKind.PropertyAccessExpression) { + const propertyAccessExpression = child as ts.PropertyAccessExpression; + const name = propertyAccessExpression.name.getText(); + const typeChecker = this.getTypeChecker(); + const type = typeChecker.getTypeAtLocation(propertyAccessExpression.expression); + + if ((name === "subscribe") && + isReferenceType(type) && + couldBeType(type.target, "Observable") && + (node.arguments.length && this.nodeIsLikelyAFunction(node.arguments[0])) && + node.arguments.length < 2 + ) { + this.addFailureAtNode(propertyAccessExpression.name, Rule.FAILURE_STRING); + } + } + }); + + super.visitCallExpression(node); + } + + private nodeIsLikelyAFunction(node: ts.Expression): boolean { + // Fast check + if (node.kind === ts.SyntaxKind.ArrowFunction || + node.kind === ts.SyntaxKind.FunctionExpression + ) { + return true; + } + // Check with a type checker + const typeChecker = this.getTypeChecker(); + const type: ts.Type = typeChecker.getTypeAtLocation(node); + return couldBeType(type, "Function") || + couldBeType(type, "ArrowFunction") || + couldBeType(type, InternalSymbolName.Function); + } +}