Skip to content

Commit 994ff25

Browse files
JasonWeinzierlJasonWeinzierl
authored
feat(ban-operators)!: type check, report in place (#25)
BREAKING CHANGE: this rule now requires typed linting. - Resolves #7 . Reworked `ban-operators` to support banning operators imported from root "rxjs" too. - This uses typed linting to actually check the operator's type instead of just reading the import statement. This was done based on the original maintainer's comments in the unit tests indicating this was necessary. - Also resolves upstream cartant#96 . Banned operators are now reported at the area they're used in the code instead of the import statement. - This allows developers to `eslint-disable-next-line` to allow banned operators more precisely. - This also resolves the only blocker before #8 can be worked. - NOTE: this only runs inside of `pipe()` due to performance reasons. - If users still want to completely ban an operator via its import, they can use the built-in ESLint rule `no-restricted-imports` (or the typescript-eslint equivalent `@typescript-eslint/no-restircted-imports`) with the `importNames` names option. - Technically users could also use `ban-observables`, since that rule doesn't distinguish between operators and static creation function exported from "rxjs"...
1 parent 02fdffd commit 994ff25

File tree

4 files changed

+129
-41
lines changed

4 files changed

+129
-41
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ The package includes the following rules.
6767
| Name                       | Description | 💼 | 🔧 | 💡 | 💭 | ❌ |
6868
| :--------------------------------------------------------------------- | :--------------------------------------------------------------------------------------------------- | :- | :- | :- | :- | :- |
6969
| [ban-observables](docs/rules/ban-observables.md) | Disallow banned observable creators. | | | | | |
70-
| [ban-operators](docs/rules/ban-operators.md) | Disallow banned operators. | | | | | |
70+
| [ban-operators](docs/rules/ban-operators.md) | Disallow banned operators. | | | | 💭 | |
7171
| [finnish](docs/rules/finnish.md) | Enforce Finnish notation. | | | | 💭 | |
7272
| [just](docs/rules/just.md) | Require the use of `just` instead of `of`. | | 🔧 | | | |
7373
| [macro](docs/rules/macro.md) | Require the use of the RxJS Tools Babel macro. | | 🔧 | | | ❌ |

‎docs/rules/ban-operators.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
# Disallow banned operators (`rxjs-x/ban-operators`)
22

3+
💭 This rule requires [type information](https://typescript-eslint.io/linting/typed-linting).
4+
35
<!-- end auto-generated rule header -->
46

57
This rule can be configured so that developers can ban any operators they want to avoid in their project.

‎src/rules/ban-operators.ts‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { AST_NODE_TYPES, TSESTree as es } from '@typescript-eslint/utils';
1+
import { TSESTree as es } from '@typescript-eslint/utils';
22
import { stripIndent } from 'common-tags';
3+
import { getTypeServices } from '../etc';
34
import { ruleCreator } from '../utils';
45

56
const defaultOptions: readonly Record<string, boolean | string>[] = [];
@@ -9,6 +10,7 @@ export const banOperatorsRule = ruleCreator({
910
meta: {
1011
docs: {
1112
description: 'Disallow banned operators.',
13+
requiresTypeChecking: true,
1214
},
1315
messages: {
1416
forbidden: 'RxJS operator is banned: {{name}}{{explanation}}.',
@@ -25,7 +27,8 @@ export const banOperatorsRule = ruleCreator({
2527
},
2628
name: 'ban-operators',
2729
create: (context) => {
28-
const bans: { explanation: string; regExp: RegExp }[] = [];
30+
const { couldBeType } = getTypeServices(context);
31+
const bans: { name: string; explanation: string }[] = [];
2932

3033
const [config] = context.options;
3134
if (!config) {
@@ -35,39 +38,34 @@ export const banOperatorsRule = ruleCreator({
3538
Object.entries(config).forEach(([key, value]) => {
3639
if (value !== false) {
3740
bans.push({
41+
name: key,
3842
explanation: typeof value === 'string' ? value : '',
39-
regExp: new RegExp(`^${key}$`),
4043
});
4144
}
4245
});
4346

44-
function getFailure(name: string) {
45-
for (let b = 0, length = bans.length; b < length; ++b) {
46-
const ban = bans[b];
47-
if (ban.regExp.test(name)) {
47+
function checkNode(node: es.Node) {
48+
for (const ban of bans) {
49+
if (couldBeType(node, ban.name, { name: /[/\\]rxjs[/\\]/ })) {
4850
const explanation = ban.explanation ? `: ${ban.explanation}` : '';
49-
return {
51+
context.report({
5052
messageId: 'forbidden',
51-
data: { name, explanation },
52-
} as const;
53+
data: { name: ban.name, explanation },
54+
node,
55+
});
56+
return;
5357
}
5458
}
55-
return undefined;
5659
}
5760

5861
return {
59-
[String.raw`ImportDeclaration[source.value=/^rxjs\u002foperators$/] > ImportSpecifier`]:
60-
(node: es.ImportSpecifier) => {
61-
const identifier = node.imported;
62-
const name = identifier.type === AST_NODE_TYPES.Identifier ? identifier.name : identifier.value;
63-
const failure = getFailure(name);
64-
if (failure) {
65-
context.report({
66-
...failure,
67-
node: identifier,
68-
});
69-
}
70-
},
62+
'CallExpression[callee.property.name=\'pipe\'] > CallExpression[callee.name]': (node: es.CallExpression) => {
63+
checkNode(node.callee);
64+
},
65+
'CallExpression[callee.property.name=\'pipe\'] > CallExpression[callee.type="MemberExpression"]': (node: es.CallExpression) => {
66+
const callee = node.callee as es.MemberExpression;
67+
checkNode(callee.property);
68+
},
7169
};
7270
},
7371
});

‎tests/rules/ban-operators.test.ts‎

Lines changed: 105 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,122 @@ import { banOperatorsRule } from '../../src/rules/ban-operators';
33
import { fromFixture } from '../etc';
44
import { ruleTester } from '../rule-tester';
55

6-
ruleTester({ types: false }).run('ban-operators', banOperatorsRule, {
6+
ruleTester({ types: true }).run('ban-operators', banOperatorsRule, {
77
valid: [
88
{
9-
code: `import { concat, merge as m, mergeMap as mm } from "rxjs/operators";`,
9+
code: stripIndent`
10+
// root import
11+
import { of, concat, merge as m, mergeMap as mm } from "rxjs";
12+
13+
of('a').pipe(concat(of('b')));
14+
of(1).pipe(m(of(2)));
15+
of('a').pipe(mm(x => of(x + '1')));
16+
`,
17+
options: [{}],
18+
},
19+
{
20+
code: stripIndent`
21+
// namespace import
22+
import * as Rx from "rxjs";
23+
24+
Rx.of('a').pipe(Rx.concat(Rx.of('b')));
25+
Rx.of(1).pipe(Rx.merge(Rx.of(2)));
26+
Rx.of('a').pipe(Rx.mergeMap(x => Rx.of(x + '1')));
27+
`,
28+
options: [{}],
1029
},
1130
{
12-
code: `import { concat, merge as m, mergeMap as mm } from "rxjs";`,
31+
code: stripIndent`
32+
// operators path import (deprecated)
33+
import { of, concat, merge as m, mergeMap as mm } from "rxjs/operators";
34+
35+
of('a').pipe(concat(of('b')));
36+
of(1).pipe(m(of(2)));
37+
of('a').pipe(mm(x => of(x + '1')));
38+
`,
39+
options: [{}],
40+
},
41+
stripIndent`
42+
// no options
43+
import { of, concat } from "rxjs";
44+
45+
of('a').pipe(concat(of('b')));
46+
`,
47+
{
48+
code: stripIndent`
49+
// non-RxJS operator
50+
import { of } from "rxjs";
51+
52+
function concat() {}
53+
54+
of('a').pipe(concat());
55+
`,
56+
options: [{ concat: true }],
1357
},
1458
{
15-
// This won't effect errors, because only imports from "rxjs/operators"
16-
// are checked. To support banning operators from "rxjs", it'll need to
17-
// check types.
18-
code: `import { concat, merge as m, mergeMap as mm } from "rxjs";`,
19-
options: [
20-
{
21-
concat: true,
22-
merge: 'because I say so',
23-
mergeMap: false,
24-
},
25-
],
59+
code: stripIndent`
60+
// only within pipe
61+
import { of, concat } from "rxjs";
62+
63+
// For performance reasons, we don't lint operators used outside of pipe.
64+
concat(of('a'));
65+
`,
66+
options: [{ concat: true }],
2667
},
2768
],
2869
invalid: [
2970
fromFixture(
3071
stripIndent`
31-
import { concat, merge as m, mergeMap as mm } from "rxjs/operators";
32-
~~~~~~ [forbidden { "name": "concat", "explanation": "" }]
33-
~~~~~ [forbidden { "name": "merge", "explanation": ": because I say so" }]
72+
// root import
73+
import { of, concat, merge as m, mergeMap as mm } from "rxjs";
74+
75+
of('a').pipe(concat(of('b')));
76+
~~~~~~ [forbidden { "name": "concat", "explanation": "" }]
77+
of(1).pipe(m(of(2)));
78+
~ [forbidden { "name": "merge", "explanation": ": because I say so" }]
79+
of('a').pipe(mm(x => of(x + '1')));
80+
`,
81+
{
82+
options: [
83+
{
84+
concat: true,
85+
merge: 'because I say so',
86+
mergeMap: false,
87+
},
88+
],
89+
},
90+
),
91+
fromFixture(
92+
stripIndent`
93+
// namespace import
94+
import * as Rx from "rxjs";
95+
96+
Rx.of('a').pipe(Rx.concat(Rx.of('b')));
97+
~~~~~~ [forbidden { "name": "concat", "explanation": "" }]
98+
Rx.of(1).pipe(Rx.merge(Rx.of(2)));
99+
~~~~~ [forbidden { "name": "merge", "explanation": "" }]
100+
Rx.of('a').pipe(Rx.mergeMap(x => Rx.of(x + '1')));
101+
`,
102+
{
103+
options: [
104+
{
105+
concat: true,
106+
merge: true,
107+
mergeMap: false,
108+
},
109+
],
110+
},
111+
),
112+
fromFixture(
113+
stripIndent`
114+
// operators path import (deprecated)
115+
import { of, concat, merge as m, mergeMap as mm } from "rxjs/operators";
116+
117+
of('a').pipe(concat(of('b')));
118+
~~~~~~ [forbidden { "name": "concat", "explanation": "" }]
119+
of(1).pipe(m(of(2)));
120+
~ [forbidden { "name": "merge", "explanation": ": because I say so" }]
121+
of('a').pipe(mm(x => of(x + '1')));
34122
`,
35123
{
36124
options: [

0 commit comments

Comments
 (0)