Skip to content

fix(no-node-access): Improve detection logic in no-node-access to resolve imported aliases and setup instances #1033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
172 changes: 172 additions & 0 deletions lib/node-utils/accessors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
import {
AST_NODE_TYPES,
ASTUtils,
type TSESTree,
} from '@typescript-eslint/utils';

/**
* A `Literal` with a `value` of type `string`.
*/
interface StringLiteral<Value extends string = string>
extends TSESTree.StringLiteral {
value: Value;
}

/**
* Checks if the given `node` is a `StringLiteral`.
*
* If a `value` is provided & the `node` is a `StringLiteral`,
* the `value` will be compared to that of the `StringLiteral`.
*
* @param {Node} node
* @param {V} [value]
*
* @return {node is StringLiteral<V>}
*
* @template V
*/
const isStringLiteral = <V extends string>(
node: TSESTree.Node,
value?: V
): node is StringLiteral<V> =>
node.type === AST_NODE_TYPES.Literal &&
typeof node.value === 'string' &&
(value === undefined || node.value === value);

interface TemplateLiteral<Value extends string = string>
extends TSESTree.TemplateLiteral {
quasis: [TSESTree.TemplateElement & { value: { raw: Value; cooked: Value } }];
}

/**
* Checks if the given `node` is a `TemplateLiteral`.
*
* Complex `TemplateLiteral`s are not considered specific, and so will return `false`.
*
* If a `value` is provided & the `node` is a `TemplateLiteral`,
* the `value` will be compared to that of the `TemplateLiteral`.
*
* @param {Node} node
* @param {V} [value]
*
* @return {node is TemplateLiteral<V>}
*
* @template V
*/
const isTemplateLiteral = <V extends string>(
node: TSESTree.Node,
value?: V
): node is TemplateLiteral<V> =>
node.type === AST_NODE_TYPES.TemplateLiteral &&
node.quasis.length === 1 && // bail out if not simple
(value === undefined || node.quasis[0].value.raw === value);

Check warning on line 62 in lib/node-utils/accessors.ts

View check run for this annotation

Codecov / codecov/patch

lib/node-utils/accessors.ts#L61-L62

Added lines #L61 - L62 were not covered by tests

export type StringNode<S extends string = string> =
| StringLiteral<S>
| TemplateLiteral<S>;

/**
* Checks if the given `node` is a {@link StringNode}.
*
* @param {Node} node
* @param {V} [specifics]
*
* @return {node is StringNode}
*
* @template V
*/
export const isStringNode = <V extends string>(
node: TSESTree.Node,
specifics?: V
): node is StringNode<V> =>
isStringLiteral(node, specifics) || isTemplateLiteral(node, specifics);

/**
* Gets the value of the given `StringNode`.
*
* If the `node` is a `TemplateLiteral`, the `raw` value is used;
* otherwise, `value` is returned instead.
*
* @param {StringNode<S>} node
*
* @return {S}
*
* @template S
*/
export const getStringValue = <S extends string>(node: StringNode<S>): S =>
isTemplateLiteral(node) ? node.quasis[0].value.raw : node.value;

/**
* An `Identifier` with a known `name` value
*/
interface KnownIdentifier<Name extends string> extends TSESTree.Identifier {
name: Name;
}

/**
* Checks if the given `node` is an `Identifier`.
*
* If a `name` is provided, & the `node` is an `Identifier`,
* the `name` will be compared to that of the `identifier`.
*
* @param {Node} node
* @param {V} [name]
*
* @return {node is KnownIdentifier<Name>}
*
* @template V
*/
export const isIdentifier = <V extends string>(
node: TSESTree.Node,
name?: V
): node is KnownIdentifier<V> =>
ASTUtils.isIdentifier(node) && (name === undefined || node.name === name);

/**
* Checks if the given `node` is a "supported accessor".
*
* This means that it's a node can be used to access properties,
* and who's "value" can be statically determined.
*
* `MemberExpression` nodes most commonly contain accessors,
* but it's possible for other nodes to contain them.
*
* If a `value` is provided & the `node` is an `AccessorNode`,
* the `value` will be compared to that of the `AccessorNode`.
*
* Note that `value` here refers to the normalised value.
* The property that holds the value is not always called `name`.
*
* @param {Node} node
* @param {V} [value]
*
* @return {node is AccessorNode<V>}
*
* @template V
*/
export const isSupportedAccessor = <V extends string>(
node: TSESTree.Node,
value?: V
): node is AccessorNode<V> =>
isIdentifier(node, value) || isStringNode(node, value);

/**
* Gets the value of the given `AccessorNode`,
* account for the different node types.
*
* @param {AccessorNode<S>} accessor
*
* @return {S}
*
* @template S
*/
export const getAccessorValue = <S extends string = string>(
accessor: AccessorNode<S>
): S =>
accessor.type === AST_NODE_TYPES.Identifier
? accessor.name
: getStringValue(accessor);

Check warning on line 168 in lib/node-utils/accessors.ts

View check run for this annotation

Codecov / codecov/patch

lib/node-utils/accessors.ts#L168

Added line #L168 was not covered by tests

export type AccessorNode<Specifics extends string = string> =
| StringNode<Specifics>
| KnownIdentifier<Specifics>;
6 changes: 6 additions & 0 deletions lib/node-utils/is-node-of-type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ export const isImportDeclaration = ASTUtils.isNodeOfType(
export const isImportDefaultSpecifier = ASTUtils.isNodeOfType(
AST_NODE_TYPES.ImportDefaultSpecifier
);
export const isTSImportEqualsDeclaration = ASTUtils.isNodeOfType(
AST_NODE_TYPES.TSImportEqualsDeclaration
);
export const isImportExpression = ASTUtils.isNodeOfType(
AST_NODE_TYPES.ImportExpression
);
export const isImportNamespaceSpecifier = ASTUtils.isNodeOfType(
AST_NODE_TYPES.ImportNamespaceSpecifier
);
Expand Down
75 changes: 59 additions & 16 deletions lib/rules/no-node-access.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
import { TSESTree, ASTUtils } from '@typescript-eslint/utils';

import { createTestingLibraryRule } from '../create-testing-library-rule';
import { isCallExpression, isMemberExpression } from '../node-utils';
import {
ALL_RETURNING_NODES,
EVENT_HANDLER_METHODS,
EVENTS_SIMULATORS,
resolveToTestingLibraryFn,
} from '../utils';

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

const ALL_PROHIBITED_MEMBERS = [
...ALL_RETURNING_NODES,
...EVENT_HANDLER_METHODS,
] as const;
const userEventInstanceNames = new Set<string>();

export default createTestingLibraryRule<Options, MessageIds>({
name: RULE_NAME,
Expand Down Expand Up @@ -65,20 +63,11 @@ export default createTestingLibraryRule<Options, MessageIds>({
? node.property.name
: null;

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

return {
CallExpression(node: TSESTree.CallExpression) {
const { callee } = node;
const property = isMemberExpression(callee) ? callee.property : null;
const object = isMemberExpression(callee) ? callee.object : null;

const propertyName = ASTUtils.isIdentifier(property)
? property.name
: null;
const objectName = ASTUtils.isIdentifier(object) ? object.name : null;

const isEventHandlerMethod = EVENT_HANDLER_METHODS.some(
(method) => method === propertyName
);
const hasUserEventInstanceName = userEventInstanceNames.has(
objectName ?? ''
);
const testingLibraryFn = resolveToTestingLibraryFn(node, context);

if (
!testingLibraryFn &&
isEventHandlerMethod &&
!hasUserEventInstanceName
) {
context.report({
node,
loc: property?.loc.start,
messageId: 'noNodeAccess',
});
}
},
VariableDeclarator(node: TSESTree.VariableDeclarator) {
const { init, id } = node;

if (!isCallExpression(init)) {
return;
}

if (
!isMemberExpression(init.callee) ||
!ASTUtils.isIdentifier(init.callee.object)
) {
return;
}

const testingLibraryFn = resolveToTestingLibraryFn(init, context);
if (
init.callee.object.name === testingLibraryFn?.local &&
ASTUtils.isIdentifier(init.callee.property) &&
init.callee.property.name === 'setup' &&
ASTUtils.isIdentifier(id)
) {
userEventInstanceNames.add(id.name);
}
},
'ExpressionStatement MemberExpression': showErrorForNodeAccess,
'VariableDeclarator MemberExpression': showErrorForNodeAccess,
};
Expand Down
1 change: 1 addition & 0 deletions lib/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export * from './compat';
export * from './file-import';
export * from './types';
export * from './resolve-to-testing-library-fn';

const combineQueries = (
variants: readonly string[],
Expand Down
Loading