From acc188552bb62cf6c6b9b3282c3cf0a546573454 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sat, 15 Jul 2017 15:04:12 +1000 Subject: [PATCH] feat(rxjs-no-subject-unsubscribe): Add rule. --- README.md | 5 +- .../async-subject-with-unsubscribe/fixture.ts | 4 + .../tsconfig.json | 13 + .../tslint.json | 8 + fixtures/subject-with-unsubscribe/fixture.ts | 4 + .../subject-with-unsubscribe/tsconfig.json | 13 + fixtures/subject-with-unsubscribe/tslint.json | 8 + .../subject-without-unsubscribe/fixture.ts | 5 + .../subject-without-unsubscribe/tsconfig.json | 13 + .../subject-without-unsubscribe/tslint.json | 8 + source/fixtures-spec.ts | 246 ++++++++++-------- source/rules/rxjsNoSubjectUnsubscribeRule.ts | 53 ++++ 12 files changed, 274 insertions(+), 106 deletions(-) create mode 100644 fixtures/async-subject-with-unsubscribe/fixture.ts create mode 100644 fixtures/async-subject-with-unsubscribe/tsconfig.json create mode 100644 fixtures/async-subject-with-unsubscribe/tslint.json create mode 100644 fixtures/subject-with-unsubscribe/fixture.ts create mode 100644 fixtures/subject-with-unsubscribe/tsconfig.json create mode 100644 fixtures/subject-with-unsubscribe/tslint.json create mode 100644 fixtures/subject-without-unsubscribe/fixture.ts create mode 100644 fixtures/subject-without-unsubscribe/tsconfig.json create mode 100644 fixtures/subject-without-unsubscribe/tslint.json create mode 100644 source/rules/rxjsNoSubjectUnsubscribeRule.ts diff --git a/README.md b/README.md index c6a53afc..6ccc3f4d 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ ### What is it? -`rxjs-tslint-rules` is small set of TSLint rules to help manage projects that use `rxjs/add/...` imports. +`rxjs-tslint-rules` is small set of TSLint rules to help manage projects that use `rxjs/add/...` imports and to highlight other potential problems. ### Why might I need it? @@ -24,7 +24,7 @@ TypeScript will see the merged declarations in all modules, making it difficult This can cause problems, as whether or not `Observable` is patched then depends upon the order in which the modules are executed. -The rules in this package can be used to highlight missing - or unused - imports. +The rules in this package can be used to highlight missing - or unused - imports and other potential problems with RxJS. ## Install @@ -54,6 +54,7 @@ The package includes the following rules: | --- | --- | --- | | `rxjs-add` | Enforces the importation of patched observables and operators used in the module. | See below | | `rxjs-no-add` | Disallows the importation of patched observables and operators. | None | +| `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-prefer-add` | Disallows the importation of `rxjs` or `rxjs/Rx`. | None | diff --git a/fixtures/async-subject-with-unsubscribe/fixture.ts b/fixtures/async-subject-with-unsubscribe/fixture.ts new file mode 100644 index 00000000..07d56420 --- /dev/null +++ b/fixtures/async-subject-with-unsubscribe/fixture.ts @@ -0,0 +1,4 @@ +import { AsyncSubject } from "rxjs/AsyncSubject"; + +const subject = new AsyncSubject(); +subject.unsubscribe(); diff --git a/fixtures/async-subject-with-unsubscribe/tsconfig.json b/fixtures/async-subject-with-unsubscribe/tsconfig.json new file mode 100644 index 00000000..09a2e6e7 --- /dev/null +++ b/fixtures/async-subject-with-unsubscribe/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/async-subject-with-unsubscribe/tslint.json b/fixtures/async-subject-with-unsubscribe/tslint.json new file mode 100644 index 00000000..a905b678 --- /dev/null +++ b/fixtures/async-subject-with-unsubscribe/tslint.json @@ -0,0 +1,8 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-no-subject-unsubscribe": { "severity": "error" } + }, + "rulesDirectory": "../../build/rules" +} diff --git a/fixtures/subject-with-unsubscribe/fixture.ts b/fixtures/subject-with-unsubscribe/fixture.ts new file mode 100644 index 00000000..e63abcd4 --- /dev/null +++ b/fixtures/subject-with-unsubscribe/fixture.ts @@ -0,0 +1,4 @@ +import { Subject } from "rxjs/Subject"; + +const subject = new Subject(); +subject.unsubscribe(); diff --git a/fixtures/subject-with-unsubscribe/tsconfig.json b/fixtures/subject-with-unsubscribe/tsconfig.json new file mode 100644 index 00000000..09a2e6e7 --- /dev/null +++ b/fixtures/subject-with-unsubscribe/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/subject-with-unsubscribe/tslint.json b/fixtures/subject-with-unsubscribe/tslint.json new file mode 100644 index 00000000..a905b678 --- /dev/null +++ b/fixtures/subject-with-unsubscribe/tslint.json @@ -0,0 +1,8 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-no-subject-unsubscribe": { "severity": "error" } + }, + "rulesDirectory": "../../build/rules" +} diff --git a/fixtures/subject-without-unsubscribe/fixture.ts b/fixtures/subject-without-unsubscribe/fixture.ts new file mode 100644 index 00000000..d985143e --- /dev/null +++ b/fixtures/subject-without-unsubscribe/fixture.ts @@ -0,0 +1,5 @@ +import { Subject } from "rxjs/Subject"; + +const subject = new Subject(); +const subscription = subject.subscribe(); +subscription.unsubscribe(); diff --git a/fixtures/subject-without-unsubscribe/tsconfig.json b/fixtures/subject-without-unsubscribe/tsconfig.json new file mode 100644 index 00000000..09a2e6e7 --- /dev/null +++ b/fixtures/subject-without-unsubscribe/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/subject-without-unsubscribe/tslint.json b/fixtures/subject-without-unsubscribe/tslint.json new file mode 100644 index 00000000..a905b678 --- /dev/null +++ b/fixtures/subject-without-unsubscribe/tslint.json @@ -0,0 +1,8 @@ +{ + "defaultSeverity": "error", + "jsRules": {}, + "rules": { + "rxjs-no-subject-unsubscribe": { "severity": "error" } + }, + "rulesDirectory": "../../build/rules" +} diff --git a/source/fixtures-spec.ts b/source/fixtures-spec.ts index eaed8ec0..951b4af7 100644 --- a/source/fixtures-spec.ts +++ b/source/fixtures-spec.ts @@ -14,179 +14,217 @@ describe("fixtures", function (): void { /*tslint:disable-next-line:no-invalid-this*/ this.timeout(5000); - describe("custom-observable", () => { + describe("import-related rules", () => { - it("should effect no errors", () => { + describe("custom-observable", () => { - const result = lint("custom-observable"); + it("should effect no errors", () => { - expect(result).to.have.property("errorCount", 0); + const result = lint("custom-observable"); + + expect(result).to.have.property("errorCount", 0); + }); }); - }); - describe("custom-operator", () => { + describe("custom-operator", () => { - it("should effect no errors", () => { + it("should effect no errors", () => { - const result = lint("custom-operator"); + const result = lint("custom-operator"); - expect(result).to.have.property("errorCount", 0); + expect(result).to.have.property("errorCount", 0); + }); }); - }); - describe("custom-source", () => { + describe("custom-source", () => { - it("should effect no errors", () => { + it("should effect no errors", () => { - const result = lint("custom-source"); + const result = lint("custom-source"); - expect(result).to.have.property("errorCount", 0); + expect(result).to.have.property("errorCount", 0); + }); }); - }); - describe("import-all", () => { + describe("import-all", () => { - it("should effect 'rxjs-prefer-add' errors", () => { + it("should effect 'rxjs-prefer-add' errors", () => { - const result = lint("import-all"); + const result = lint("import-all"); - expect(result).to.have.property("errorCount", 2); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-prefer-add"); - expect(result.failures[1]).to.have.property("ruleName", "rxjs-prefer-add"); + expect(result).to.have.property("errorCount", 2); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-prefer-add"); + expect(result.failures[1]).to.have.property("ruleName", "rxjs-prefer-add"); + }); }); - }); - describe("no-add", () => { + describe("no-add", () => { - it("should effect 'rxjs-no-add' errors", () => { + it("should effect 'rxjs-no-add' errors", () => { - const result = lint("no-add", "tslint.json"); + const result = lint("no-add", "tslint.json"); - expect(result).to.have.property("errorCount", 1); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-add"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-add"); + }); }); - }); - describe("no-errors", () => { + describe("no-errors", () => { - it("should effect no errors", () => { + it("should effect no errors", () => { - const result = lint("no-errors"); + const result = lint("no-errors"); - expect(result).to.have.property("errorCount", 0); + expect(result).to.have.property("errorCount", 0); + }); }); - }); - describe("no-errors-with-file", () => { + describe("no-errors-with-file", () => { - it("should effect no errors", () => { + it("should effect no errors", () => { - const result = lint("no-errors-with-file", "tslint.json"); + const result = lint("no-errors-with-file", "tslint.json"); - expect(result).to.have.property("errorCount", 0); + expect(result).to.have.property("errorCount", 0); + }); }); - }); - describe("no-observable", () => { + describe("no-observable", () => { - it("should effect 'rxjs-add' errors", () => { + it("should effect 'rxjs-add' errors", () => { - const result = lint("no-observable"); + const result = lint("no-observable"); - expect(result).to.have.property("errorCount", 9); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[6]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[7]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[8]).to.have.property("ruleName", "rxjs-add"); + expect(result).to.have.property("errorCount", 9); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[6]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[7]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[8]).to.have.property("ruleName", "rxjs-add"); + }); }); - }); - describe("no-observable-with-file", () => { + describe("no-observable-with-file", () => { - it("should effect 'rxjs-add' errors", () => { + it("should effect 'rxjs-add' errors", () => { - const result = lint("no-observable-with-file", "tslint.json"); + const result = lint("no-observable-with-file", "tslint.json"); - expect(result).to.have.property("errorCount", 9); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[6]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[7]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[8]).to.have.property("ruleName", "rxjs-add"); + expect(result).to.have.property("errorCount", 9); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[6]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[7]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[8]).to.have.property("ruleName", "rxjs-add"); + }); }); - }); - describe("no-operator", () => { + describe("no-operator", () => { - it("should effect 'rxjs-add' errors", () => { + it("should effect 'rxjs-add' errors", () => { - const result = lint("no-operator"); + const result = lint("no-operator"); - expect(result).to.have.property("errorCount", 6); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + expect(result).to.have.property("errorCount", 6); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + }); }); - }); - describe("no-operator-with-file", () => { + describe("no-operator-with-file", () => { - it("should effect 'rxjs-add' errors", () => { + it("should effect 'rxjs-add' errors", () => { - const result = lint("no-operator-with-file", "tslint.json"); + const result = lint("no-operator-with-file", "tslint.json"); - expect(result).to.have.property("errorCount", 6); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); - expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + expect(result).to.have.property("errorCount", 6); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[1]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[2]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[3]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[4]).to.have.property("ruleName", "rxjs-add"); + expect(result.failures[5]).to.have.property("ruleName", "rxjs-add"); + }); }); - }); - describe("observable-create", () => { + describe("observable-create", () => { - it("should effect an error unless explicit typing is used", () => { + it("should effect an error unless explicit typing is used", () => { - const result = lint("observable-create"); + const result = lint("observable-create"); - expect(result).to.have.property("errorCount", 1); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + }); }); - }); - describe("unused-observable", () => { + describe("unused-observable", () => { - it("should effect 'rxjs-no-unused-add' errors", () => { + it("should effect 'rxjs-no-unused-add' errors", () => { - const result = lint("unused-observable"); + const result = lint("unused-observable"); - expect(result).to.have.property("errorCount", 1); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + }); + }); + + describe("unused-operator", () => { + + it("should effect 'rxjs-no-unused-add' errors", () => { + + const result = lint("unused-operator"); + + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + }); }); }); - describe("unused-operator", () => { + describe("subject-related rules", () => { + + describe("async-subject-with-unsubscribe", () => { + + it("should effect an 'rxjs-no-subject-unsubscribe' error", () => { + + const result = lint("async-subject-with-unsubscribe", "tslint.json"); + + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-subject-unsubscribe"); + }); + }); + + describe("subject-with-unsubscribe", () => { + + it("should effect an 'rxjs-no-subject-unsubscribe' error", () => { + + const result = lint("subject-with-unsubscribe", "tslint.json"); + + expect(result).to.have.property("errorCount", 1); + expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-subject-unsubscribe"); + }); + }); + + describe("subject-without-unsubscribe", () => { - it("should effect 'rxjs-no-unused-add' errors", () => { + it("should not effect an 'rxjs-no-subject-unsubscribe' error", () => { - const result = lint("unused-operator"); + const result = lint("subject-without-unsubscribe", "tslint.json"); - expect(result).to.have.property("errorCount", 1); - expect(result.failures[0]).to.have.property("ruleName", "rxjs-no-unused-add"); + expect(result).to.have.property("errorCount", 0); + }); }); }); diff --git a/source/rules/rxjsNoSubjectUnsubscribeRule.ts b/source/rules/rxjsNoSubjectUnsubscribeRule.ts new file mode 100644 index 00000000..9ce19e2e --- /dev/null +++ b/source/rules/rxjsNoSubjectUnsubscribeRule.ts @@ -0,0 +1,53 @@ +/** + * @license Copyright © 2017 Nicholas Jamieson. 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"; + +export class Rule extends Lint.Rules.TypedRule { + + public static metadata: Lint.IRuleMetadata = { + description: "Disallows the calling of unsubscribe directly upon subject instances.", + options: null, + optionsDescription: "Not configurable.", + requiresTypeInfo: true, + ruleName: "rxjs-no-subject-unsubscribe", + type: "functionality", + typescriptOnly: true + }; + + public static FAILURE_STRING = "Calling unsubscribe on subjects 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 === "unsubscribe") && isReferenceType(type) && couldBeType(type.target, "Subject")) { + this.addFailureAtNode(propertyAccessExpression.name, Rule.FAILURE_STRING); + } + } + }); + + super.visitCallExpression(node); + } +}