Skip to content

Commit 7812dae

Browse files
y-hsgwy-hsgw
authored
fix(no-node-access): improve detection for Testing Library calls (#1033)
Fixes #1024 Fixes #1032 Improves the accuracy of the rule when detecting if import aliases and `setup` instances belong to Testing Library or not.
1 parent b1a4da7 commit 7812dae

File tree

7 files changed

+877
-35
lines changed

7 files changed

+877
-35
lines changed

‎lib/node-utils/accessors.ts

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
import {
2+
AST_NODE_TYPES,
3+
ASTUtils,
4+
type TSESTree,
5+
} from '@typescript-eslint/utils';
6+
7+
import { isLiteral, isTemplateLiteral } from './is-node-of-type';
8+
9+
/**
10+
* A `Literal` with a `value` of type `string`.
11+
*/
12+
interface StringLiteral<Value extends string = string>
13+
extends TSESTree.StringLiteral {
14+
value: Value;
15+
}
16+
17+
/**
18+
* Checks if the given `node` is a `StringLiteral`.
19+
*
20+
* If a `value` is provided & the `node` is a `StringLiteral`,
21+
* the `value` will be compared to that of the `StringLiteral`.
22+
*/
23+
const isStringLiteral = <V extends string>(
24+
node: TSESTree.Node,
25+
value?: V
26+
): node is StringLiteral<V> =>
27+
isLiteral(node) &&
28+
typeof node.value === 'string' &&
29+
(value === undefined || node.value === value);
30+
31+
interface TemplateLiteral<Value extends string = string>
32+
extends TSESTree.TemplateLiteral {
33+
quasis: [TSESTree.TemplateElement & { value: { raw: Value; cooked: Value } }];
34+
}
35+
36+
/**
37+
* Checks if the given `node` is a `TemplateLiteral`.
38+
*
39+
* Complex `TemplateLiteral`s are not considered specific, and so will return `false`.
40+
*
41+
* If a `value` is provided & the `node` is a `TemplateLiteral`,
42+
* the `value` will be compared to that of the `TemplateLiteral`.
43+
*/
44+
const isSimpleTemplateLiteral = <V extends string>(
45+
node: TSESTree.Node,
46+
value?: V
47+
): node is TemplateLiteral<V> =>
48+
isTemplateLiteral(node) &&
49+
node.quasis.length === 1 && // bail out if not simple
50+
(value === undefined || node.quasis[0].value.raw === value);
51+
52+
export type StringNode<S extends string = string> =
53+
| StringLiteral<S>
54+
| TemplateLiteral<S>;
55+
56+
/**
57+
* Checks if the given `node` is a {@link StringNode}.
58+
*/
59+
export const isStringNode = <V extends string>(
60+
node: TSESTree.Node,
61+
specifics?: V
62+
): node is StringNode<V> =>
63+
isStringLiteral(node, specifics) || isSimpleTemplateLiteral(node, specifics);
64+
65+
/**
66+
* Gets the value of the given `StringNode`.
67+
*
68+
* If the `node` is a `TemplateLiteral`, the `raw` value is used;
69+
* otherwise, `value` is returned instead.
70+
*/
71+
export const getStringValue = <S extends string>(node: StringNode<S>): S =>
72+
isSimpleTemplateLiteral(node) ? node.quasis[0].value.raw : node.value;
73+
74+
/**
75+
* An `Identifier` with a known `name` value
76+
*/
77+
interface KnownIdentifier<Name extends string> extends TSESTree.Identifier {
78+
name: Name;
79+
}
80+
81+
/**
82+
* Checks if the given `node` is an `Identifier`.
83+
*
84+
* If a `name` is provided, & the `node` is an `Identifier`,
85+
* the `name` will be compared to that of the `identifier`.
86+
*/
87+
export const isIdentifier = <V extends string>(
88+
node: TSESTree.Node,
89+
name?: V
90+
): node is KnownIdentifier<V> =>
91+
ASTUtils.isIdentifier(node) && (name === undefined || node.name === name);
92+
93+
/**
94+
* Checks if the given `node` is a "supported accessor".
95+
*
96+
* This means that it's a node can be used to access properties,
97+
* and who's "value" can be statically determined.
98+
*
99+
* `MemberExpression` nodes most commonly contain accessors,
100+
* but it's possible for other nodes to contain them.
101+
*
102+
* If a `value` is provided & the `node` is an `AccessorNode`,
103+
* the `value` will be compared to that of the `AccessorNode`.
104+
*
105+
* Note that `value` here refers to the normalised value.
106+
* The property that holds the value is not always called `name`.
107+
*/
108+
export const isSupportedAccessor = <V extends string>(
109+
node: TSESTree.Node,
110+
value?: V
111+
): node is AccessorNode<V> =>
112+
isIdentifier(node, value) || isStringNode(node, value);
113+
114+
/**
115+
* Gets the value of the given `AccessorNode`,
116+
* account for the different node types.
117+
*/
118+
export const getAccessorValue = <S extends string = string>(
119+
accessor: AccessorNode<S>
120+
): S =>
121+
accessor.type === AST_NODE_TYPES.Identifier
122+
? accessor.name
123+
: getStringValue(accessor);
124+
125+
export type AccessorNode<Specifics extends string = string> =
126+
| StringNode<Specifics>
127+
| KnownIdentifier<Specifics>;

‎lib/node-utils/is-node-of-type.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,12 @@ export const isImportDeclaration = ASTUtils.isNodeOfType(
3030
export const isImportDefaultSpecifier = ASTUtils.isNodeOfType(
3131
AST_NODE_TYPES.ImportDefaultSpecifier
3232
);
33+
export const isTSImportEqualsDeclaration = ASTUtils.isNodeOfType(
34+
AST_NODE_TYPES.TSImportEqualsDeclaration
35+
);
36+
export const isImportExpression = ASTUtils.isNodeOfType(
37+
AST_NODE_TYPES.ImportExpression
38+
);
3339
export const isImportNamespaceSpecifier = ASTUtils.isNodeOfType(
3440
AST_NODE_TYPES.ImportNamespaceSpecifier
3541
);
@@ -40,6 +46,9 @@ export const isJSXAttribute = ASTUtils.isNodeOfType(
4046
AST_NODE_TYPES.JSXAttribute
4147
);
4248
export const isLiteral = ASTUtils.isNodeOfType(AST_NODE_TYPES.Literal);
49+
export const isTemplateLiteral = ASTUtils.isNodeOfType(
50+
AST_NODE_TYPES.TemplateLiteral
51+
);
4352
export const isMemberExpression = ASTUtils.isNodeOfType(
4453
AST_NODE_TYPES.MemberExpression
4554
);

‎lib/rules/no-node-access.ts

Lines changed: 59 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,18 @@
11
import { TSESTree, ASTUtils } from '@typescript-eslint/utils';
22

33
import { createTestingLibraryRule } from '../create-testing-library-rule';
4+
import { isCallExpression, isMemberExpression } from '../node-utils';
45
import {
56
ALL_RETURNING_NODES,
67
EVENT_HANDLER_METHODS,
7-
EVENTS_SIMULATORS,
8+
resolveToTestingLibraryFn,
89
} from '../utils';
910

1011
export const RULE_NAME = 'no-node-access';
1112
export type MessageIds = 'noNodeAccess';
1213
export type Options = [{ allowContainerFirstChild: boolean }];
1314

14-
const ALL_PROHIBITED_MEMBERS = [
15-
...ALL_RETURNING_NODES,
16-
...EVENT_HANDLER_METHODS,
17-
] as const;
15+
const userEventInstanceNames = new Set<string>();
1816

1917
export default createTestingLibraryRule<Options, MessageIds>({
2018
name: RULE_NAME,
@@ -65,20 +63,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
6563
? node.property.name
6664
: null;
6765

68-
const objectName = ASTUtils.isIdentifier(node.object)
69-
? node.object.name
70-
: null;
7166
if (
7267
propertyName &&
73-
ALL_PROHIBITED_MEMBERS.some(
68+
ALL_RETURNING_NODES.some(
7469
(allReturningNode) => allReturningNode === propertyName
75-
) &&
76-
![
77-
...EVENTS_SIMULATORS,
78-
// TODO: As discussed in https://github.com/testing-library/eslint-plugin-testing-library/issues/1024, this is just a temporary workaround.
79-
// We should address the root cause and implement a proper solution instead of explicitly excluding 'user' here.
80-
'user',
81-
].some((simulator) => simulator === objectName)
70+
)
8271
) {
8372
if (allowContainerFirstChild && propertyName === 'firstChild') {
8473
return;
@@ -100,6 +89,60 @@ export default createTestingLibraryRule<Options, MessageIds>({
10089
}
10190

10291
return {
92+
CallExpression(node: TSESTree.CallExpression) {
93+
const { callee } = node;
94+
const property = isMemberExpression(callee) ? callee.property : null;
95+
const object = isMemberExpression(callee) ? callee.object : null;
96+
97+
const propertyName = ASTUtils.isIdentifier(property)
98+
? property.name
99+
: null;
100+
const objectName = ASTUtils.isIdentifier(object) ? object.name : null;
101+
102+
const isEventHandlerMethod = EVENT_HANDLER_METHODS.some(
103+
(method) => method === propertyName
104+
);
105+
const hasUserEventInstanceName = userEventInstanceNames.has(
106+
objectName ?? ''
107+
);
108+
const testingLibraryFn = resolveToTestingLibraryFn(node, context);
109+
110+
if (
111+
!testingLibraryFn &&
112+
isEventHandlerMethod &&
113+
!hasUserEventInstanceName
114+
) {
115+
context.report({
116+
node,
117+
loc: property?.loc.start,
118+
messageId: 'noNodeAccess',
119+
});
120+
}
121+
},
122+
VariableDeclarator(node: TSESTree.VariableDeclarator) {
123+
const { init, id } = node;
124+
125+
if (!isCallExpression(init)) {
126+
return;
127+
}
128+
129+
if (
130+
!isMemberExpression(init.callee) ||
131+
!ASTUtils.isIdentifier(init.callee.object)
132+
) {
133+
return;
134+
}
135+
136+
const testingLibraryFn = resolveToTestingLibraryFn(init, context);
137+
if (
138+
init.callee.object.name === testingLibraryFn?.local &&
139+
ASTUtils.isIdentifier(init.callee.property) &&
140+
init.callee.property.name === 'setup' &&
141+
ASTUtils.isIdentifier(id)
142+
) {
143+
userEventInstanceNames.add(id.name);
144+
}
145+
},
103146
'ExpressionStatement MemberExpression': showErrorForNodeAccess,
104147
'VariableDeclarator MemberExpression': showErrorForNodeAccess,
105148
};

‎lib/utils/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
export * from './compat';
22
export * from './file-import';
33
export * from './types';
4+
export * from './resolve-to-testing-library-fn';
45

56
const combineQueries = (
67
variants: readonly string[],
@@ -30,6 +31,8 @@ const LIBRARY_MODULES = [
3031
'@marko/testing-library',
3132
] as const;
3233

34+
const USER_EVENT_MODULE = '@testing-library/user-event';
35+
3336
const SYNC_QUERIES_VARIANTS = [
3437
'getBy',
3538
'getAllBy',
@@ -150,4 +153,5 @@ export {
150153
PRESENCE_MATCHERS,
151154
ABSENCE_MATCHERS,
152155
EVENT_HANDLER_METHODS,
156+
USER_EVENT_MODULE,
153157
};

0 commit comments

Comments
 (0)