From 6cc1d61bc2b4a1c1459fbf01d43754941c6b5c63 Mon Sep 17 00:00:00 2001 From: Dane Vanderbilt Date: Mon, 19 Aug 2024 15:01:41 -0500 Subject: [PATCH 1/4] feat: Add `no-subscribe-in-pipe` rule to forbid calling `subscribe` within `pipe` operators --- src/rules/no-subscribe-in-pipe.ts | 68 ++++++++++ tests/rules/no-subscribe-in-pipe.ts | 198 ++++++++++++++++++++++++++++ 2 files changed, 266 insertions(+) create mode 100644 src/rules/no-subscribe-in-pipe.ts create mode 100644 tests/rules/no-subscribe-in-pipe.ts diff --git a/src/rules/no-subscribe-in-pipe.ts b/src/rules/no-subscribe-in-pipe.ts new file mode 100644 index 00000000..e6f104cf --- /dev/null +++ b/src/rules/no-subscribe-in-pipe.ts @@ -0,0 +1,68 @@ +/** + * @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/eslint-plugin-rxjs + */ + +import { TSESTree as es } from "@typescript-eslint/experimental-utils"; +import { getParent, getTypeServices } from "eslint-etc"; +import { ruleCreator } from "../utils"; + +const rule = ruleCreator({ + defaultOptions: [], + meta: { + docs: { + description: + "Forbids the calling of `subscribe` within any RxJS operator inside a `pipe`.", + recommended: "error", + }, + fixable: undefined, + hasSuggestions: false, + messages: { + forbidden: "Subscribe calls within pipe operators are forbidden.", + }, + schema: [], + type: "problem", + }, + name: "no-subscribe-in-pipe", + create: (context) => { + const { couldBeObservable, couldBeType } = getTypeServices(context); + + function isWithinPipe(node: es.Node): boolean { + let parent = getParent(node); + while (parent) { + if ( + parent.type === "CallExpression" && + parent.callee.type === "MemberExpression" && + parent.callee.property.type === "Identifier" && + parent.callee.property.name === "pipe" + ) { + return true; + } + parent = getParent(parent); + } + return false; + } + + return { + "CallExpression > MemberExpression[property.name='subscribe']": ( + node: es.MemberExpression + ) => { + if ( + !couldBeObservable(node.object) && + !couldBeType(node.object, "Subscribable") + ) { + return; + } + + if (isWithinPipe(node)) { + context.report({ + messageId: "forbidden", + node: node.property, + }); + } + }, + }; + }, +}); + +export = rule; diff --git a/tests/rules/no-subscribe-in-pipe.ts b/tests/rules/no-subscribe-in-pipe.ts new file mode 100644 index 00000000..99401e8a --- /dev/null +++ b/tests/rules/no-subscribe-in-pipe.ts @@ -0,0 +1,198 @@ +import { stripIndent } from "common-tags"; +import { fromFixture } from "eslint-etc"; +import rule = require("../../source/rules/no-subscribe-in-pipe"); +import { ruleTester } from "../utils"; + +ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { + valid: [ + stripIndent` + // subscribe outside of pipe + import { of } from "rxjs"; + of(47).subscribe(value => { + console.log(value); + }); + `, + stripIndent` + // pipe without subscribe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => x * 2) + ).subscribe(value => { + console.log(value); + }); + `, + stripIndent` + // nested pipes without subscribe + import { of } from "rxjs"; + import { map, mergeMap } from "rxjs/operators"; + of(47).pipe( + mergeMap(x => of(x).pipe( + map(y => y * 2) + )) + ).subscribe(value => { + console.log(value); + }); + `, + stripIndent` + // subscribe in a function outside pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + const logValue = (value) => of(value).subscribe(console.log); + of(47).pipe( + map(x => x * 2) + ).subscribe(logValue); + `, + stripIndent` + // subscribe method on a non-Observable object inside pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + const customObject = { subscribe: () => {} }; + of(47).pipe( + map(x => { + customObject.subscribe(); + return x * 2; + }) + ).subscribe(); + `, + stripIndent` + // subscribe as a variable name inside pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => { + const subscribe = 5; + return x * subscribe; + }) + ).subscribe(); + `, + stripIndent` + // subscribe in a comment inside pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => { + // of(x).subscribe(console.log); + return x * 2; + }) + ).subscribe(); + `, + stripIndent` + // subscribe as a string inside pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => { + console.log("subscribe"); + return x * 2; + }) + ).subscribe(); + `, + ], + invalid: [ + fromFixture( + stripIndent` + // subscribe inside map operator + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => { + of(x).subscribe(console.log); + ~~~~~~~~~ [forbidden] + return x * 2; + }) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe inside mergeMap operator + import { of } from "rxjs"; + import { mergeMap } from "rxjs/operators"; + of(47).pipe( + mergeMap(x => of(x).pipe( + map(y => { + of(y).subscribe(console.log); + ~~~~~~~~~ [forbidden] + return y * 2; + }) + )) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe inside tap operator + import { of } from "rxjs"; + import { tap } from "rxjs/operators"; + of(47).pipe( + tap(x => { + of(x).subscribe(console.log); + ~~~~~~~~~ [forbidden] + }) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe inside switchMap operator + import { of } from "rxjs"; + import { switchMap } from "rxjs/operators"; + of(47).pipe( + switchMap(x => { + of(x).subscribe(console.log); + ~~~~~~~~~ [forbidden] + return of(x * 2); + }) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe inside nested pipes + import { of } from "rxjs"; + import { map, mergeMap } from "rxjs/operators"; + of(47).pipe( + mergeMap(x => of(x).pipe( + map(y => { + of(y).subscribe(console.log); + ~~~~~~~~~ [forbidden] + return y * 2; + }) + )) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe inside a deeply nested function in pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => { + const nestedFunc = () => { + const deeplyNested = () => { + of(x).subscribe(console.log); + ~~~~~~~~~ [forbidden] + }; + deeplyNested(); + }; + nestedFunc(); + return x * 2; + }) + ).subscribe(); + ` + ), + fromFixture( + stripIndent` + // subscribe in a ternary operator in pipe + import { of } from "rxjs"; + import { map } from "rxjs/operators"; + of(47).pipe( + map(x => x > 50 ? x : of(x).subscribe(console.log)) + ~~~~~~~~~ [forbidden] + ).subscribe(); + ` + ), + ], +}); From 7448e4971ed1c2dd9bbb691760c15853d75b8154 Mon Sep 17 00:00:00 2001 From: Dane Vanderbilt Date: Mon, 19 Aug 2024 19:54:29 -0500 Subject: [PATCH 2/4] chore: Add docs and readme link --- docs/rules/no-subscribe-in-pipe.md | 35 ++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 docs/rules/no-subscribe-in-pipe.md diff --git a/docs/rules/no-subscribe-in-pipe.md b/docs/rules/no-subscribe-in-pipe.md new file mode 100644 index 00000000..5244faee --- /dev/null +++ b/docs/rules/no-subscribe-in-pipe.md @@ -0,0 +1,35 @@ +# Avoid `subscribe` calls inside `pipe` operators (`no-subscribe-in-pipe`) + +This rule effects failures if `subscribe` is called within any operator inside a `pipe` operation. + +## Rule details + +Examples of **incorrect** code for this rule: + +```ts +import { of } from "rxjs"; +import { map } from "rxjs/operators"; + +of(42, 54).pipe( + map(value => { + of(value).subscribe(console.log); // This will trigger the rule + return value * 2; + }) +).subscribe(result => console.log(result)); +``` + +Examples of **correct** code for this rule: + +```ts +import { of } from "rxjs"; +import { map, tap } from "rxjs/operators"; + +of(42, 54).pipe( + tap(value => console.log(value)), + map(value => value * 2) +).subscribe(result => console.log(result)); +``` + +## Options + +This rule has no options. \ No newline at end of file From 3a064cce0d9c47cec2ff0497946f1ed45d057cce Mon Sep 17 00:00:00 2001 From: Dane Vanderbilt Date: Tue, 3 Dec 2024 21:15:31 -0600 Subject: [PATCH 3/4] Making requested changes --- README.md | 1 + docs/rules/no-subscribe-in-pipe.md | 12 +++-- src/configs/recommended.ts | 1 + src/configs/strict.ts | 1 + src/index.ts | 2 + src/rules/no-subscribe-in-pipe.ts | 49 +++++++++---------- ...n-pipe.ts => no-subscribe-in-pipe.test.ts} | 32 +++++++----- 7 files changed, 54 insertions(+), 44 deletions(-) rename tests/rules/{no-subscribe-in-pipe.ts => no-subscribe-in-pipe.test.ts} (89%) diff --git a/README.md b/README.md index 71f80f61..56b8cbc3 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,7 @@ The package includes the following rules. | [no-subject-unsubscribe](docs/rules/no-subject-unsubscribe.md) | Disallow calling the `unsubscribe` method of subjects. | ✅ 🔒 | | | 💭 | | | [no-subject-value](docs/rules/no-subject-value.md) | Disallow accessing the `value` property of a `BehaviorSubject` instance. | | | | 💭 | | | [no-subscribe-handlers](docs/rules/no-subscribe-handlers.md) | Disallow passing handlers to `subscribe`. | | | | 💭 | | +| [no-subscribe-in-pipe](docs/rules/no-subscribe-in-pipe.md) | Disallow calling of `subscribe` within any RxJS operator inside a `pipe`. | ✅ 🔒 | | | 💭 | | | [no-tap](docs/rules/no-tap.md) | Disallow the `tap` operator. | | | | | ❌ | | [no-topromise](docs/rules/no-topromise.md) | Disallow use of the `toPromise` method. | ✅ 🔒 | | 💡 | 💭 | | | [no-unbound-methods](docs/rules/no-unbound-methods.md) | Disallow passing unbound methods. | ✅ 🔒 | | | 💭 | | diff --git a/docs/rules/no-subscribe-in-pipe.md b/docs/rules/no-subscribe-in-pipe.md index 5244faee..24a6352c 100644 --- a/docs/rules/no-subscribe-in-pipe.md +++ b/docs/rules/no-subscribe-in-pipe.md @@ -1,4 +1,10 @@ -# Avoid `subscribe` calls inside `pipe` operators (`no-subscribe-in-pipe`) +# Disallow calling of `subscribe` within any RxJS operator inside a `pipe` (`rxjs-x/no-subscribe-in-pipe`) + +💼 This rule is enabled in the following configs: ✅ `recommended`, 🔒 `strict`. + +💭 This rule requires [type information](https://typescript-eslint.io/linting/typed-linting). + + This rule effects failures if `subscribe` is called within any operator inside a `pipe` operation. @@ -29,7 +35,3 @@ of(42, 54).pipe( map(value => value * 2) ).subscribe(result => console.log(result)); ``` - -## Options - -This rule has no options. \ No newline at end of file diff --git a/src/configs/recommended.ts b/src/configs/recommended.ts index c29e47a7..5d08cb94 100644 --- a/src/configs/recommended.ts +++ b/src/configs/recommended.ts @@ -20,6 +20,7 @@ export const createRecommendedConfig = ( 'rxjs-x/no-redundant-notify': 'error', 'rxjs-x/no-sharereplay': 'error', 'rxjs-x/no-subject-unsubscribe': 'error', + 'rxjs-x/no-subscribe-in-pipe': 'error', 'rxjs-x/no-topromise': 'error', 'rxjs-x/no-unbound-methods': 'error', 'rxjs-x/no-unsafe-subject-next': 'error', diff --git a/src/configs/strict.ts b/src/configs/strict.ts index aca1e482..259ca297 100644 --- a/src/configs/strict.ts +++ b/src/configs/strict.ts @@ -29,6 +29,7 @@ export const createStrictConfig = ( 'rxjs-x/no-sharereplay': 'error', 'rxjs-x/no-subclass': 'error', 'rxjs-x/no-subject-unsubscribe': 'error', + 'rxjs-x/no-subscribe-in-pipe': 'error', 'rxjs-x/no-topromise': 'error', 'rxjs-x/no-unbound-methods': 'error', 'rxjs-x/no-unsafe-subject-next': 'error', diff --git a/src/index.ts b/src/index.ts index 3bab9b8d..cf693fa2 100644 --- a/src/index.ts +++ b/src/index.ts @@ -35,6 +35,7 @@ import { noSubclassRule } from './rules/no-subclass'; import { noSubjectUnsubscribeRule } from './rules/no-subject-unsubscribe'; import { noSubjectValueRule } from './rules/no-subject-value'; import { noSubscribeHandlersRule } from './rules/no-subscribe-handlers'; +import { noSubscribeInPipeRule } from './rules/no-subscribe-in-pipe'; import { noTapRule } from './rules/no-tap'; import { noTopromiseRule } from './rules/no-topromise'; import { noUnboundMethodsRule } from './rules/no-unbound-methods'; @@ -83,6 +84,7 @@ const plugin = { 'no-subject-unsubscribe': noSubjectUnsubscribeRule, 'no-subject-value': noSubjectValueRule, 'no-subscribe-handlers': noSubscribeHandlersRule, + 'no-subscribe-in-pipe': noSubscribeInPipeRule, 'no-tap': noTapRule, 'no-topromise': noTopromiseRule, 'no-unbound-methods': noUnboundMethodsRule, diff --git a/src/rules/no-subscribe-in-pipe.ts b/src/rules/no-subscribe-in-pipe.ts index e6f104cf..bf9fe1b2 100644 --- a/src/rules/no-subscribe-in-pipe.ts +++ b/src/rules/no-subscribe-in-pipe.ts @@ -1,62 +1,59 @@ -/** - * @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/eslint-plugin-rxjs - */ +import { AST_NODE_TYPES, TSESTree as es } from '@typescript-eslint/utils'; +import { getTypeServices } from '../etc'; +import { ruleCreator } from '../utils'; -import { TSESTree as es } from "@typescript-eslint/experimental-utils"; -import { getParent, getTypeServices } from "eslint-etc"; -import { ruleCreator } from "../utils"; - -const rule = ruleCreator({ +export const noSubscribeInPipeRule = ruleCreator({ defaultOptions: [], meta: { docs: { description: - "Forbids the calling of `subscribe` within any RxJS operator inside a `pipe`.", - recommended: "error", + 'Disallow calling of `subscribe` within any RxJS operator inside a `pipe`.', + recommended: 'recommended', + requiresTypeChecking: true, }, fixable: undefined, hasSuggestions: false, messages: { - forbidden: "Subscribe calls within pipe operators are forbidden.", + forbidden: 'Subscribe calls within pipe operators are forbidden.', }, schema: [], - type: "problem", + type: 'problem', }, - name: "no-subscribe-in-pipe", + name: 'no-subscribe-in-pipe', create: (context) => { const { couldBeObservable, couldBeType } = getTypeServices(context); function isWithinPipe(node: es.Node): boolean { - let parent = getParent(node); + let parent = node.parent; + while (parent) { if ( - parent.type === "CallExpression" && - parent.callee.type === "MemberExpression" && - parent.callee.property.type === "Identifier" && - parent.callee.property.name === "pipe" + parent.type === AST_NODE_TYPES.CallExpression + && parent.callee.type === AST_NODE_TYPES.MemberExpression + && parent.callee.property.type === AST_NODE_TYPES.Identifier + && parent.callee.property.name === 'pipe' ) { return true; } - parent = getParent(parent); + parent = node.parent; } return false; } return { - "CallExpression > MemberExpression[property.name='subscribe']": ( - node: es.MemberExpression + 'CallExpression > MemberExpression[property.name=\'subscribe\']': ( + node: es.MemberExpression, ) => { if ( - !couldBeObservable(node.object) && - !couldBeType(node.object, "Subscribable") + !couldBeObservable(node.object) + && !couldBeType(node.object, 'Subscribable') ) { return; } if (isWithinPipe(node)) { context.report({ - messageId: "forbidden", + messageId: 'forbidden', node: node.property, }); } @@ -64,5 +61,3 @@ const rule = ruleCreator({ }; }, }); - -export = rule; diff --git a/tests/rules/no-subscribe-in-pipe.ts b/tests/rules/no-subscribe-in-pipe.test.ts similarity index 89% rename from tests/rules/no-subscribe-in-pipe.ts rename to tests/rules/no-subscribe-in-pipe.test.ts index 99401e8a..78a126ae 100644 --- a/tests/rules/no-subscribe-in-pipe.ts +++ b/tests/rules/no-subscribe-in-pipe.test.ts @@ -1,9 +1,9 @@ -import { stripIndent } from "common-tags"; -import { fromFixture } from "eslint-etc"; -import rule = require("../../source/rules/no-subscribe-in-pipe"); -import { ruleTester } from "../utils"; +import { stripIndent } from 'common-tags'; +import { noSubscribeInPipeRule } from '../../src/rules/no-subscribe-in-pipe'; +import { fromFixture } from '../etc'; +import { ruleTester } from '../rule-tester'; -ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { +ruleTester({ types: true }).run('no-subscribe-in-pipe', noSubscribeInPipeRule, { valid: [ stripIndent` // subscribe outside of pipe @@ -88,6 +88,14 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { }) ).subscribe(); `, + stripIndent` + // subscribe inside of an Observable constructor + import { Observable, of } from "rxjs"; + + new Observable(subscriber => { + of(42).subscribe(subscriber); + }).subscribe(); + `, ], invalid: [ fromFixture( @@ -102,7 +110,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { return x * 2; }) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -118,7 +126,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { }) )) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -131,7 +139,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { ~~~~~~~~~ [forbidden] }) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -145,7 +153,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { return of(x * 2); }) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -161,7 +169,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { }) )) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -181,7 +189,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { return x * 2; }) ).subscribe(); - ` + `, ), fromFixture( stripIndent` @@ -192,7 +200,7 @@ ruleTester({ types: true }).run("no-subscribe-in-pipe", rule, { map(x => x > 50 ? x : of(x).subscribe(console.log)) ~~~~~~~~~ [forbidden] ).subscribe(); - ` + `, ), ], }); From 150d2350df96b0d9073bce77e4132e922a4d2d1b Mon Sep 17 00:00:00 2001 From: Dane Vanderbilt Date: Wed, 4 Dec 2024 09:49:51 -0600 Subject: [PATCH 4/4] Update src/rules/no-subscribe-in-pipe.ts Co-authored-by: Jason Weinzierl Signed-off-by: Dane Vanderbilt --- src/rules/no-subscribe-in-pipe.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/no-subscribe-in-pipe.ts b/src/rules/no-subscribe-in-pipe.ts index bf9fe1b2..8f008fb1 100644 --- a/src/rules/no-subscribe-in-pipe.ts +++ b/src/rules/no-subscribe-in-pipe.ts @@ -35,7 +35,7 @@ export const noSubscribeInPipeRule = ruleCreator({ ) { return true; } - parent = node.parent; + parent = parent.parent; } return false; }