Skip to content

Commit f3ab04d

Browse files
fix(no-async-subscribe): report async next properties on observer object (#214)
Adds support for catching `next: async () => ...` properties when passing an object into `subscribe`. This may cause a significant increase in reports for this rule, because previously we were not checking observer objects at all and `prefer-observer` may have been pushing codebases to migrate to observer objects. So this also enhances the `forbidden` message to include a snippet from this rule's docs, so that intellisense is more useful. Resolves #163
1 parent 087bf3c commit f3ab04d

File tree

3 files changed

+109
-20
lines changed

3 files changed

+109
-20
lines changed

docs/rules/no-async-subscribe.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,14 @@ of(42).subscribe(async value => {
2323
const data2 = await fetch(`https://api.some.com/things/${data1.id}`);
2424
console.log(data2);
2525
});
26+
27+
of(42).subscribe({
28+
next: async () => {
29+
const data1 = await fetch(`https://api.some.com/things/${value}`);
30+
const data2 = await fetch(`https://api.some.com/things/${data1.id}`);
31+
console.log(data2);
32+
},
33+
})
2634
```
2735

2836
Examples of **correct** code for this rule:
@@ -34,6 +42,11 @@ of(42).pipe(
3442
switchMap(value => fetch(`http://api.some.com/things/${value}`)),
3543
switchMap(data1 => fetch(`http://api.some.com/things/${data1.id}`)),
3644
).subscribe(data2 => console.log(data2));
45+
46+
of(42).pipe(
47+
switchMap(value => fetch(`http://api.some.com/things/${value}`)),
48+
switchMap(data1 => fetch(`http://api.some.com/things/${data1.id}`)),
49+
).subscribe({ next: data2 => console.log(data2) });
3750
```
3851

3952
## When Not To Use It

src/rules/no-async-subscribe.ts

Lines changed: 45 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { TSESTree as es } from '@typescript-eslint/utils';
2-
import { getTypeServices } from '../etc';
2+
import { getTypeServices, isArrowFunctionExpression, isFunctionExpression } from '../etc';
33
import { ruleCreator } from '../utils';
44

55
export const noAsyncSubscribeRule = ruleCreator({
@@ -11,7 +11,7 @@ export const noAsyncSubscribeRule = ruleCreator({
1111
requiresTypeChecking: true,
1212
},
1313
messages: {
14-
forbidden: 'Passing async functions to subscribe is forbidden.',
14+
forbidden: 'Passing async functions to subscribe is forbidden. Use operators like `concatMap` or `switchMap` to avoid race conditions.',
1515
},
1616
schema: [],
1717
type: 'problem',
@@ -20,34 +20,59 @@ export const noAsyncSubscribeRule = ruleCreator({
2020
create: (context) => {
2121
const { couldBeObservable } = getTypeServices(context);
2222

23-
function checkNode(
23+
function report(
2424
node: es.FunctionExpression | es.ArrowFunctionExpression,
2525
) {
26-
const parentNode = node.parent as es.CallExpression;
26+
const { loc } = node;
27+
// only report the `async` keyword
28+
const asyncLoc = {
29+
...loc,
30+
end: {
31+
...loc.start,
32+
column: loc.start.column + 5,
33+
},
34+
};
35+
36+
context.report({
37+
messageId: 'forbidden',
38+
loc: asyncLoc,
39+
});
40+
}
41+
42+
function checkSingleNext(
43+
funcNode: es.FunctionExpression | es.ArrowFunctionExpression,
44+
) {
45+
const parentNode = funcNode.parent as es.CallExpression;
2746
const callee = parentNode.callee as es.MemberExpression;
2847

2948
if (couldBeObservable(callee.object)) {
30-
const { loc } = node;
31-
// only report the `async` keyword
32-
const asyncLoc = {
33-
...loc,
34-
end: {
35-
...loc.start,
36-
column: loc.start.column + 5,
37-
},
38-
};
39-
40-
context.report({
41-
messageId: 'forbidden',
42-
loc: asyncLoc,
43-
});
49+
report(funcNode);
4450
}
4551
}
52+
53+
function checkObserverObjectProp(
54+
propNode: es.Property,
55+
) {
56+
const parentNode = propNode.parent.parent as es.CallExpression;
57+
const callee = parentNode.callee as es.MemberExpression;
58+
59+
if (couldBeObservable(callee.object)
60+
&& (isArrowFunctionExpression(propNode.value)
61+
|| isFunctionExpression(propNode.value))
62+
) {
63+
report(propNode.value);
64+
}
65+
}
66+
4667
return {
4768
'CallExpression[callee.property.name=\'subscribe\'] > FunctionExpression[async=true]':
48-
checkNode,
69+
checkSingleNext,
4970
'CallExpression[callee.property.name=\'subscribe\'] > ArrowFunctionExpression[async=true]':
50-
checkNode,
71+
checkSingleNext,
72+
'CallExpression[callee.property.name=\'subscribe\'] > ObjectExpression > Property[key.name=\'next\'][value.type="ArrowFunctionExpression"][value.async=true]':
73+
checkObserverObjectProp,
74+
'CallExpression[callee.property.name=\'subscribe\'] > ObjectExpression > Property[key.name=\'next\'][value.type="FunctionExpression"][value.async=true]':
75+
checkObserverObjectProp,
5176
};
5277
},
5378
});

tests/rules/no-async-subscribe.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,31 @@ ruleTester({ types: true }).run('no-async-subscribe', noAsyncSubscribeRule, {
3333
};
3434
whatever.subscribe(async () => { await 42; });
3535
`,
36+
stripIndent`
37+
// observer object
38+
import { of } from "rxjs";
39+
40+
of('a').subscribe({
41+
next: () => {},
42+
});
43+
of('a').subscribe({
44+
next: function() {},
45+
});
46+
`,
47+
stripIndent`
48+
// non-RxJS observer object
49+
const whatever = {
50+
subscribe: (observer: {
51+
next?: (value: unknown) => void;
52+
}) => {},
53+
};
54+
whatever.subscribe({
55+
next: () => {},
56+
});
57+
whatever.subscribe({
58+
next: function() {},
59+
});
60+
`,
3661
],
3762
invalid: [
3863
fromFixture(
@@ -46,6 +71,19 @@ ruleTester({ types: true }).run('no-async-subscribe', noAsyncSubscribeRule, {
4671
});
4772
`,
4873
),
74+
fromFixture(
75+
stripIndent`
76+
// async arrow function; observer object
77+
import { of } from "rxjs";
78+
79+
of("a").subscribe({
80+
next: async () => {
81+
~~~~~ [forbidden]
82+
return await "a";
83+
},
84+
});
85+
`,
86+
),
4987
fromFixture(
5088
stripIndent`
5189
// async function
@@ -57,5 +95,18 @@ ruleTester({ types: true }).run('no-async-subscribe', noAsyncSubscribeRule, {
5795
});
5896
`,
5997
),
98+
fromFixture(
99+
stripIndent`
100+
// async function; observer object
101+
import { of } from "rxjs";
102+
103+
of("a").subscribe({
104+
next: async function() {
105+
~~~~~ [forbidden]
106+
return await "a";
107+
},
108+
});
109+
`,
110+
),
60111
],
61112
});

0 commit comments

Comments
 (0)