Skip to content

Commit 782f823

Browse files
feat(no-ignored-subscription): ignore for certain operators (#187)
This adds new options and default behavior to `no-ignored-subscription` so that operators like `takeUntil` or `first` will silence this rule. The main new option, `completers`, specifies which operators should silence this rule because they complete the observable automatically. By default, we've set `completers` to allow `takeUntil`, `takeWhile`, `take`, `first`, `last`, and Angular's `takeUntilDestroyed`. Additionally, we are using the same logic that's used for `no-unsafe-takeuntil`: if you place another operator _after_ one of these "completers", then this rule will consider the subscription not properly handled and will trigger. The other new option, `postCompleters`, specifies which operators can be placed _after_ a "completer" and still prevent this rule from triggering. By default, we've set `postCompleters` to the same set of defaults used by `no-unsafe-takeuntil` (things like `share`, `finalize`, etc). Example of new allowed functionality: ```ts import { BehaviorSubject, first } from 'rxjs'; const foo$ = new BehaviorSubject('foo'); foo$ .pipe(first()) .subscribe(() => console.log('first!')); // Logs 'first!' and then unsubscribes. ``` Example of more complicated functionality that still doesn't trigger this rule: ```ts import { interval, Subject, takeUntil, share } from 'rxjs'; const stop$ = new Subject(); interval(1000) .pipe( takeUntil(stop$), max(), ).subscribe(x => console.log('max: ', x)); stop$.next(); // Logs the last value of the interval at the time when you called stop$ and then unsubscribes. ``` Note: there's relevant upstream conversation here: cartant#39 . The original author rejected this feature, but I found that re-using `no-unsafe-takeuntil`'s algorithm for "what is a valid last operator" was straightforward. I'm open to feedback if there's serious overlooked edge cases, but it seems like a workable solution for simple cases. Additional work in this PR: refactored `utils.ts` into separate files with a `utils/index.ts` entrypoint. Resolves #60
1 parent 8c858bb commit 782f823

File tree

11 files changed

+300
-109
lines changed

11 files changed

+300
-109
lines changed

docs/rules/no-ignored-subscription.md

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,11 @@
44

55
<!-- end auto-generated rule header -->
66

7-
The effects failures if an subscription returned by call to `subscribe` is neither assigned to a variable or property or passed to a function.
7+
This rule effects failures if an subscription returned by call to `subscribe` is neither assigned to a variable or property or passed to a function.
8+
9+
This rule is aware of operators which can automatically complete the observable (see the rule's options, below)
10+
and will not effect failures if they are present in the `pipe` before calling `subscribe`.
11+
(Note this does not work if you compose an observable into a variable and subscribe later.)
812

913
## Rule details
1014

@@ -24,6 +28,14 @@ const subscription = interval(1e3).subscribe(
2428
);
2529
```
2630

31+
Operators that automatically unsubscribe will let the subscription be ignored:
32+
33+
```ts
34+
interval(1e3).pipe(take(3)).subscribe(
35+
(value) => console.log(value)
36+
);
37+
```
38+
2739
When subscribers are passed to `subscribe` they are chained, so the returned subscription can be ignored:
2840

2941
```ts
@@ -32,11 +44,23 @@ const numbers = new Observable<number>(subscriber => {
3244
});
3345
```
3446

47+
## Options
48+
49+
<!-- begin auto-generated rule options list -->
50+
51+
| Name | Description | Type | Default |
52+
| :--------------- | :---------------------------------------------------------------------------------- | :------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
53+
| `completers` | An array of operator names that will complete the observable and silence this rule. | String[] | [`takeUntil`, `takeWhile`, `take`, `first`, `last`, `takeUntilDestroyed`] |
54+
| `postCompleters` | An array of operator names that are allowed to follow the completion operators. | String[] | [`count`, `defaultIfEmpty`, `endWith`, `every`, `finalize`, `finally`, `isEmpty`, `last`, `max`, `min`, `publish`, `publishBehavior`, `publishLast`, `publishReplay`, `reduce`, `share`, `shareReplay`, `skipLast`, `takeLast`, `throwIfEmpty`, `toArray`] |
55+
56+
<!-- end auto-generated rule options list -->
57+
3558
## When Not To Use It
3659

3760
If you don't care about unsubscribing from all observables in your project, then you may not need this rule.
38-
Alternatively, your project might use operators like `take`, `takeUntil`, `takeWhile`, etc.
39-
or Angular's `takeUntilDestroyed` to automatically handle subscriptions.
61+
Alternatively, your project might compose observables with operators like `take`, `takeUntil`, `takeWhile`, etc.
62+
or [Angular's `takeUntilDestroyed`](https://angular.dev/api/core/rxjs-interop/takeUntilDestroyed)
63+
and then call `subscribe` elsewhere, which cannot be detected by this rule.
4064

4165
Type checked lint rules are more powerful than traditional lint rules, but also require configuring type checked linting.
4266

src/constants.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,29 @@
11
export const defaultObservable = String.raw`[Aa]ction(s|s\$|\$)$`;
2+
3+
/**
4+
* The names of operators that are safe to be used after
5+
* operators like `takeUntil` that complete the observable.
6+
*/
7+
export const DEFAULT_VALID_POST_COMPLETION_OPERATORS = [
8+
'count',
9+
'defaultIfEmpty',
10+
'endWith',
11+
'every',
12+
'finalize',
13+
'finally',
14+
'isEmpty',
15+
'last',
16+
'max',
17+
'min',
18+
'publish',
19+
'publishBehavior',
20+
'publishLast',
21+
'publishReplay',
22+
'reduce',
23+
'share',
24+
'shareReplay',
25+
'skipLast',
26+
'takeLast',
27+
'throwIfEmpty',
28+
'toArray',
29+
];

src/rules/no-ignored-subscription.ts

Lines changed: 59 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,16 @@
11
import { TSESTree as es } from '@typescript-eslint/utils';
2-
import { getTypeServices } from '../etc';
3-
import { ruleCreator } from '../utils';
2+
import { stripIndent } from 'common-tags';
3+
import { DEFAULT_VALID_POST_COMPLETION_OPERATORS } from '../constants';
4+
import { getTypeServices, isCallExpression, isIdentifier, isMemberExpression } from '../etc';
5+
import { findIsLastOperatorOrderValid, ruleCreator } from '../utils';
6+
7+
const defaultOptions: readonly {
8+
completers?: string[];
9+
postCompleters?: string[];
10+
}[] = [];
411

512
export const noIgnoredSubscriptionRule = ruleCreator({
6-
defaultOptions: [],
13+
defaultOptions,
714
meta: {
815
docs: {
916
description: 'Disallow ignoring the subscription returned by `subscribe`.',
@@ -12,13 +19,56 @@ export const noIgnoredSubscriptionRule = ruleCreator({
1219
messages: {
1320
forbidden: 'Ignoring returned subscriptions is forbidden.',
1421
},
15-
schema: [],
22+
schema: [
23+
{
24+
properties: {
25+
completers: { type: 'array', items: { type: 'string' }, description: 'An array of operator names that will complete the observable and silence this rule.', default: ['takeUntil', 'takeWhile', 'take', 'first', 'last', 'takeUntilDestroyed'] },
26+
postCompleters: { type: 'array', items: { type: 'string' }, description: 'An array of operator names that are allowed to follow the completion operators.', default: DEFAULT_VALID_POST_COMPLETION_OPERATORS },
27+
},
28+
type: 'object',
29+
description: stripIndent`
30+
An object with optional \`completers\` and \`postCompleters\` properties.
31+
The \`completers\` property is an array containing the names of operators that will complete the observable and silence this rule.
32+
The \`postCompleters\` property is an array containing the names of the operators that are allowed to follow the completion operators.
33+
`,
34+
},
35+
],
1636
type: 'problem',
1737
},
1838
name: 'no-ignored-subscription',
1939
create: (context) => {
40+
const [config = {}] = context.options;
41+
const {
42+
completers = ['takeUntil', 'takeWhile', 'take', 'first', 'last', 'takeUntilDestroyed'],
43+
postCompleters = DEFAULT_VALID_POST_COMPLETION_OPERATORS,
44+
} = config;
45+
46+
const checkedOperatorsRegExp = new RegExp(
47+
`^(${completers.join('|')})$`,
48+
);
49+
2050
const { couldBeObservable, couldBeType } = getTypeServices(context);
2151

52+
function hasAllowedOperator(node: es.Node): boolean {
53+
if (
54+
isCallExpression(node)
55+
&& isMemberExpression(node.callee)
56+
&& isIdentifier(node.callee.property)
57+
&& node.callee.property.name === 'pipe'
58+
) {
59+
// Only ignore the subscription if an allowed completion operator is last (excluding allowed post-completion operators).
60+
const { isOrderValid, operatorNode } = findIsLastOperatorOrderValid(
61+
node,
62+
checkedOperatorsRegExp,
63+
postCompleters,
64+
);
65+
66+
return isOrderValid && !!operatorNode;
67+
}
68+
69+
return false;
70+
}
71+
2272
return {
2373
'ExpressionStatement > CallExpression > MemberExpression[property.name=\'subscribe\']':
2474
(node: es.MemberExpression) => {
@@ -30,6 +80,11 @@ export const noIgnoredSubscriptionRule = ruleCreator({
3080
) {
3181
return;
3282
}
83+
84+
if (hasAllowedOperator(node.object)) {
85+
return;
86+
}
87+
3388
context.report({
3489
messageId: 'forbidden',
3590
node: node.property,

src/rules/no-unsafe-takeuntil.ts

Lines changed: 15 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,16 @@
11
import { TSESTree as es } from '@typescript-eslint/utils';
22
import { stripIndent } from 'common-tags';
3+
import { DEFAULT_VALID_POST_COMPLETION_OPERATORS } from '../constants';
34
import {
45
getTypeServices,
5-
isCallExpression,
6-
isIdentifier,
7-
isMemberExpression,
86
} from '../etc';
9-
import { ruleCreator } from '../utils';
7+
import { findIsLastOperatorOrderValid, ruleCreator } from '../utils';
108

119
const defaultOptions: readonly {
1210
alias?: string[];
1311
allow?: string[];
1412
}[] = [];
1513

16-
const allowedOperators = [
17-
'count',
18-
'defaultIfEmpty',
19-
'endWith',
20-
'every',
21-
'finalize',
22-
'finally',
23-
'isEmpty',
24-
'last',
25-
'max',
26-
'min',
27-
'publish',
28-
'publishBehavior',
29-
'publishLast',
30-
'publishReplay',
31-
'reduce',
32-
'share',
33-
'shareReplay',
34-
'skipLast',
35-
'takeLast',
36-
'throwIfEmpty',
37-
'toArray',
38-
];
39-
4014
export const noUnsafeTakeuntilRule = ruleCreator({
4115
defaultOptions,
4216
meta: {
@@ -52,7 +26,7 @@ export const noUnsafeTakeuntilRule = ruleCreator({
5226
{
5327
properties: {
5428
alias: { type: 'array', items: { type: 'string' }, description: 'An array of operator names that should be treated similarly to `takeUntil`.' },
55-
allow: { type: 'array', items: { type: 'string' }, description: 'An array of operator names that are allowed to follow `takeUntil`.', default: allowedOperators },
29+
allow: { type: 'array', items: { type: 'string' }, description: 'An array of operator names that are allowed to follow `takeUntil`.', default: DEFAULT_VALID_POST_COMPLETION_OPERATORS },
5630
},
5731
type: 'object',
5832
description: stripIndent`
@@ -67,7 +41,7 @@ export const noUnsafeTakeuntilRule = ruleCreator({
6741
create: (context) => {
6842
let checkedOperatorsRegExp = /^takeUntil$/;
6943
const [config = {}] = context.options;
70-
const { alias, allow = allowedOperators } = config;
44+
const { alias, allow = DEFAULT_VALID_POST_COMPLETION_OPERATORS } = config;
7145

7246
if (alias) {
7347
checkedOperatorsRegExp = new RegExp(
@@ -86,44 +60,18 @@ export const noUnsafeTakeuntilRule = ruleCreator({
8660
return;
8761
}
8862

89-
type State = 'allowed' | 'disallowed' | 'taken';
90-
91-
pipeCallExpression.arguments.reduceRight((state, arg) => {
92-
if (state === 'taken') {
93-
return state;
94-
}
95-
96-
if (!isCallExpression(arg)) {
97-
return 'disallowed';
98-
}
99-
100-
let operatorName: string;
101-
if (isIdentifier(arg.callee)) {
102-
operatorName = arg.callee.name;
103-
} else if (
104-
isMemberExpression(arg.callee)
105-
&& isIdentifier(arg.callee.property)
106-
) {
107-
operatorName = arg.callee.property.name;
108-
} else {
109-
return 'disallowed';
110-
}
111-
112-
if (checkedOperatorsRegExp.test(operatorName)) {
113-
if (state === 'disallowed') {
114-
context.report({
115-
messageId: 'forbidden',
116-
node: arg.callee,
117-
});
118-
}
119-
return 'taken';
120-
}
63+
const { isOrderValid, operatorNode } = findIsLastOperatorOrderValid(
64+
pipeCallExpression,
65+
checkedOperatorsRegExp,
66+
allow,
67+
);
12168

122-
if (!allow.includes(operatorName)) {
123-
return 'disallowed';
124-
}
125-
return state;
126-
}, 'allowed' as State);
69+
if (!isOrderValid && operatorNode) {
70+
context.report({
71+
messageId: 'forbidden',
72+
node: operatorNode,
73+
});
74+
}
12775
}
12876

12977
return {

src/utils.ts

Lines changed: 0 additions & 35 deletions
This file was deleted.
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
export function createRegExpForWords(
2+
config: string | string[],
3+
): RegExp | undefined {
4+
if (!config?.length) {
5+
return undefined;
6+
}
7+
const flags = 'i';
8+
if (typeof config === 'string') {
9+
return new RegExp(config, flags);
10+
}
11+
const words = config;
12+
const joined = words.map((word) => String.raw`(\b|_)${word}(\b|_)`).join('|');
13+
return new RegExp(`(${joined})`, flags);
14+
}

src/utils/escape-regexp.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
export function escapeRegExp(text: string): string {
2+
// https://stackoverflow.com/a/3561711/6680611
3+
return text.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');
4+
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { TSESTree as es } from '@typescript-eslint/utils';
2+
import { isCallExpression, isIdentifier, isMemberExpression } from '../etc';
3+
4+
/**
5+
* Finds the last operator in an observable composition matching the given regex
6+
* which is ordered such that only the given {@link allow} operators
7+
* are present after it.
8+
*/
9+
export function findIsLastOperatorOrderValid(pipeCallExpression: es.CallExpression, operatorsRegExp: RegExp, allow: string[]) {
10+
let isOrderValid = true;
11+
let operatorNode: es.Node | undefined;
12+
13+
for (let i = pipeCallExpression.arguments.length - 1; i >= 0; i--) {
14+
const arg = pipeCallExpression.arguments[i];
15+
16+
if (operatorNode) {
17+
break;
18+
}
19+
20+
if (!isCallExpression(arg)) {
21+
isOrderValid = false;
22+
continue;
23+
}
24+
25+
let operatorName: string;
26+
if (isIdentifier(arg.callee)) {
27+
operatorName = arg.callee.name;
28+
} else if (
29+
isMemberExpression(arg.callee)
30+
&& isIdentifier(arg.callee.property)
31+
) {
32+
operatorName = arg.callee.property.name;
33+
} else {
34+
isOrderValid = false;
35+
continue;
36+
}
37+
38+
if (operatorsRegExp.test(operatorName)) {
39+
operatorNode = arg.callee;
40+
break;
41+
}
42+
43+
if (!allow.includes(operatorName)) {
44+
isOrderValid = false;
45+
continue;
46+
}
47+
}
48+
49+
return { isOrderValid, operatorNode };
50+
}

0 commit comments

Comments
 (0)