Skip to content

Commit dafc378

Browse files
JasonWeinzierlJasonWeinzierl
authored
feat(no-sharereplay-before-takeuntil): new rule (#241)
New rule to catch putting `shareReplay` before `takeUntil` with `refCount: false`. * This rule is added to the strict config. * This is using the same algorithm as `no-unsafe-takeuntil` to determine if takeUntil itself is in a valid position; this rule will not trigger if takeUntil is invalid. * This rule also cannot handle complex scenarios like composing multiple observables or `refCount` set to a variable with unknown boolean value. Resolves #108
1 parent cfe4211 commit dafc378

File tree

6 files changed

+321
-47
lines changed

6 files changed

+321
-47
lines changed

‎README.md‎

Lines changed: 48 additions & 47 deletions
Large diffs are not rendered by default.

‎docs/rules/no-sharereplay-before-takeuntil.md‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# Disallow using `shareReplay({ refCount: false })` before `takeUntil` (`rxjs-x/no-sharereplay-before-takeuntil`)
2+
3+
💼 This rule is enabled in the 🔒 `strict` config.
4+
5+
<!-- end auto-generated rule header -->
6+
7+
This rule effects failures if the `shareReplay` operator is used without its reference counting behavior
8+
and placed in an observable composition before `takeUntil`.
9+
10+
## Rule details
11+
12+
Examples of **incorrect** code for this rule:
13+
14+
```ts
15+
source.pipe(
16+
shareReplay(),
17+
takeUntil(notifier),
18+
);
19+
```
20+
21+
Examples of **correct** code for this rule:
22+
23+
```ts
24+
source.pipe(
25+
shareReplay({ refCount: true }),
26+
takeUntil(notifier),
27+
);
28+
```
29+
30+
## When Not To Use It
31+
32+
If you are confident your project uses `shareReplay` and `takeUntil` correctly,
33+
then you may not need this rule.
34+
35+
## Further reading
36+
37+
- [Avoiding takeUntil leaks#An update](https://ncjamieson.com/avoiding-takeuntil-leaks/#an-update)
38+
39+
## Resources
40+
41+
- [Rule source](/src/rules/no-sharereplay-before-takeuntil.ts)
42+
- [Test source](/tests/rules/no-sharereplay-before-takeuntil.test.ts)

‎src/configs/strict.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export const createStrictConfig = (
2626
'rxjs-x/no-misused-observables': 'error',
2727
'rxjs-x/no-nested-subscribe': 'error',
2828
'rxjs-x/no-redundant-notify': 'error',
29+
'rxjs-x/no-sharereplay-before-takeuntil': 'error',
2930
'rxjs-x/no-sharereplay': 'error',
3031
'rxjs-x/no-subclass': 'error',
3132
'rxjs-x/no-subject-unsubscribe': 'error',

‎src/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import { noMisusedObservablesRule } from './rules/no-misused-observables';
3232
import { noNestedSubscribeRule } from './rules/no-nested-subscribe';
3333
import { noRedundantNotifyRule } from './rules/no-redundant-notify';
3434
import { noSharereplayRule } from './rules/no-sharereplay';
35+
import { noSharereplayBeforeTakeuntilRule } from './rules/no-sharereplay-before-takeuntil';
3536
import { noSubclassRule } from './rules/no-subclass';
3637
import { noSubjectUnsubscribeRule } from './rules/no-subject-unsubscribe';
3738
import { noSubjectValueRule } from './rules/no-subject-value';
@@ -79,6 +80,7 @@ const allRules = {
7980
'no-nested-subscribe': noNestedSubscribeRule,
8081
'no-redundant-notify': noRedundantNotifyRule,
8182
'no-sharereplay': noSharereplayRule,
83+
'no-sharereplay-before-takeuntil': noSharereplayBeforeTakeuntilRule,
8284
'no-subclass': noSubclassRule,
8385
'no-subject-unsubscribe': noSubjectUnsubscribeRule,
8486
'no-subject-value': noSubjectValueRule,

‎src/rules/no-sharereplay-before-takeuntil.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { TSESTree as es } from '@typescript-eslint/utils';
2+
import { DEFAULT_VALID_POST_COMPLETION_OPERATORS } from '../constants';
3+
import { isIdentifier, isLiteral, isMemberExpression, isObjectExpression, isProperty } from '../etc';
4+
import { findIsLastOperatorOrderValid, ruleCreator } from '../utils';
5+
6+
export const noSharereplayBeforeTakeuntilRule = ruleCreator({
7+
defaultOptions: [],
8+
meta: {
9+
docs: {
10+
description: 'Disallow using `shareReplay({ refCount: false })` before `takeUntil`.',
11+
recommended: 'strict',
12+
},
13+
messages: {
14+
forbidden: 'shareReplay before takeUntil is forbidden unless \'refCount: true\' is specified.',
15+
},
16+
schema: [],
17+
type: 'problem',
18+
},
19+
name: 'no-sharereplay-before-takeuntil',
20+
create: (context) => {
21+
function checkCallExpression(node: es.CallExpression) {
22+
const pipeCallExpression = node.parent as es.CallExpression;
23+
if (
24+
!pipeCallExpression.arguments
25+
|| !isMemberExpression(pipeCallExpression.callee)
26+
|| !isIdentifier(pipeCallExpression.callee.property)
27+
|| pipeCallExpression.callee.property.name !== 'pipe'
28+
) {
29+
return;
30+
}
31+
32+
const { isOrderValid, operatorNode: takeUntilNode } = findIsLastOperatorOrderValid(
33+
pipeCallExpression,
34+
/^takeUntil$/,
35+
DEFAULT_VALID_POST_COMPLETION_OPERATORS,
36+
);
37+
if (!isOrderValid || !takeUntilNode) {
38+
// takeUntil is not present or in an unsafe position itself.
39+
return;
40+
}
41+
42+
if (takeUntilNode.range[0] < node.range[0]) {
43+
// takeUntil is before shareReplay.
44+
return;
45+
}
46+
47+
const shareReplayConfig = node.arguments[0];
48+
if (
49+
!shareReplayConfig
50+
|| !isObjectExpression(shareReplayConfig)
51+
) {
52+
// refCount defaults to false if no config is provided.
53+
context.report({
54+
messageId: 'forbidden',
55+
node: node.callee,
56+
});
57+
return;
58+
}
59+
60+
const refCountElement = shareReplayConfig.properties
61+
.filter(isProperty)
62+
.find(prop =>
63+
isIdentifier(prop.key)
64+
&& prop.key.name === 'refCount');
65+
if (
66+
!refCountElement
67+
|| (isLiteral(refCountElement.value)
68+
&& refCountElement.value.value === false)
69+
) {
70+
context.report({
71+
messageId: 'forbidden',
72+
node: node.callee,
73+
});
74+
}
75+
}
76+
77+
return {
78+
'CallExpression[callee.name="shareReplay"]': (node: es.CallExpression) => {
79+
checkCallExpression(node);
80+
},
81+
'CallExpression[callee.property.name="shareReplay"]': (node: es.CallExpression) => {
82+
checkCallExpression(node);
83+
},
84+
};
85+
},
86+
});

‎tests/rules/no-sharereplay-before-takeuntil.test.ts‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
import { stripIndent } from 'common-tags';
2+
import { noSharereplayBeforeTakeuntilRule } from '../../src/rules/no-sharereplay-before-takeuntil';
3+
import { fromFixture } from '../etc';
4+
import { ruleTester } from '../rule-tester';
5+
6+
ruleTester({ types: false }).run('no-sharereplay-before-takeuntil', noSharereplayBeforeTakeuntilRule, {
7+
valid: [
8+
stripIndent`
9+
// with refCount true
10+
import { of, takeUntil, shareReplay } from "rxjs";
11+
12+
const a = of("a");
13+
const b = of("b");
14+
15+
a.pipe(shareReplay({ refCount: true }), takeUntil(b));
16+
`,
17+
stripIndent`
18+
// with refCount true as const
19+
import { of, takeUntil, shareReplay } from "rxjs";
20+
21+
const a = of("a");
22+
const b = of("b");
23+
24+
const t = true as const;
25+
a.pipe(shareReplay({ refCount: t }), takeUntil(b));
26+
`,
27+
stripIndent`
28+
// refCount as variable not supported
29+
import { of, takeUntil, shareReplay } from "rxjs";
30+
31+
const a = of("a");
32+
const b = of("b");
33+
34+
const t = false;
35+
a.pipe(shareReplay({ refCount: t }), takeUntil(b));
36+
`,
37+
stripIndent`
38+
// composed observables not supported
39+
import { of, takeUntil, shareReplay } from "rxjs";
40+
41+
const a = of("a");
42+
const b = of("b");
43+
44+
const sr = shareReplay({ refCount: false });
45+
a.pipe(sr, takeUntil(b));
46+
`,
47+
stripIndent`
48+
// shareReplay after takeUntil
49+
import { of, takeUntil, shareReplay } from "rxjs";
50+
51+
const a = of("a");
52+
const b = of("b");
53+
54+
a.pipe(takeUntil(b), shareReplay({ refCount: false }));
55+
`,
56+
stripIndent`
57+
// shareReplay after takeUntil with operators in between
58+
import { of, takeUntil, shareReplay, map, filter } from "rxjs";
59+
60+
const a = of("a");
61+
const b = of("b");
62+
63+
a.pipe(takeUntil(b), map(x => x), filter(x => !!x), shareReplay());
64+
`,
65+
stripIndent`
66+
// namespace import with refCount true
67+
import * as Rx from "rxjs";
68+
69+
const a = Rx.of("a");
70+
const b = Rx.of("b");
71+
72+
a.pipe(Rx.shareReplay({ refCount: true }), Rx.takeUntil(b));
73+
`,
74+
stripIndent`
75+
// shareReplay by itself
76+
import { of, shareReplay } from "rxjs";
77+
78+
const a = of("a");
79+
80+
a.pipe(shareReplay());
81+
`,
82+
stripIndent`
83+
// unrelated
84+
import { of, takeUntil, toArray } from "rxjs";
85+
86+
const a = of("a");
87+
const b = of("b");
88+
89+
a.pipe(takeUntil(b), toArray());
90+
`,
91+
],
92+
invalid: [
93+
fromFixture(
94+
stripIndent`
95+
// without config
96+
import { of, takeUntil, shareReplay } from "rxjs";
97+
98+
const a = of("a");
99+
const b = of("b");
100+
101+
a.pipe(shareReplay(), takeUntil(b));
102+
~~~~~~~~~~~ [forbidden]
103+
`,
104+
),
105+
fromFixture(
106+
stripIndent`
107+
// with refCount false
108+
import { of, takeUntil, shareReplay } from "rxjs";
109+
110+
const a = of("a");
111+
const b = of("b");
112+
113+
a.pipe(shareReplay({ refCount: false }), takeUntil(b));
114+
~~~~~~~~~~~ [forbidden]
115+
`,
116+
),
117+
fromFixture(
118+
stripIndent`
119+
// with operators in between
120+
import { of, takeUntil, shareReplay, map, filter } from "rxjs";
121+
122+
const a = of("a");
123+
const b = of("b");
124+
125+
a.pipe(shareReplay(), map(x => x), filter(x => !!x), takeUntil(b));
126+
~~~~~~~~~~~ [forbidden]
127+
`,
128+
),
129+
fromFixture(
130+
stripIndent`
131+
// namespace import
132+
import * as Rx from "rxjs";
133+
134+
const a = Rx.of("a");
135+
const b = Rx.of("b");
136+
137+
a.pipe(Rx.shareReplay(), Rx.takeUntil(b));
138+
~~~~~~~~~~~~~~ [forbidden]
139+
`,
140+
),
141+
],
142+
});

0 commit comments

Comments
 (0)