diff --git a/lib/node-utils/accessors.ts b/lib/node-utils/accessors.ts new file mode 100644 index 00000000..1d50c4fb --- /dev/null +++ b/lib/node-utils/accessors.ts @@ -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 + 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} + * + * @template V + */ +const isStringLiteral = ( + node: TSESTree.Node, + value?: V +): node is StringLiteral => + node.type === AST_NODE_TYPES.Literal && + typeof node.value === 'string' && + (value === undefined || node.value === value); + +interface TemplateLiteral + 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} + * + * @template V + */ +const isTemplateLiteral = ( + node: TSESTree.Node, + value?: V +): node is TemplateLiteral => + node.type === AST_NODE_TYPES.TemplateLiteral && + node.quasis.length === 1 && // bail out if not simple + (value === undefined || node.quasis[0].value.raw === value); + +export type StringNode = + | StringLiteral + | TemplateLiteral; + +/** + * Checks if the given `node` is a {@link StringNode}. + * + * @param {Node} node + * @param {V} [specifics] + * + * @return {node is StringNode} + * + * @template V + */ +export const isStringNode = ( + node: TSESTree.Node, + specifics?: V +): node is StringNode => + 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} node + * + * @return {S} + * + * @template S + */ +export const getStringValue = (node: StringNode): S => + isTemplateLiteral(node) ? node.quasis[0].value.raw : node.value; + +/** + * An `Identifier` with a known `name` value + */ +interface KnownIdentifier 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} + * + * @template V + */ +export const isIdentifier = ( + node: TSESTree.Node, + name?: V +): node is KnownIdentifier => + 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} + * + * @template V + */ +export const isSupportedAccessor = ( + node: TSESTree.Node, + value?: V +): node is AccessorNode => + isIdentifier(node, value) || isStringNode(node, value); + +/** + * Gets the value of the given `AccessorNode`, + * account for the different node types. + * + * @param {AccessorNode} accessor + * + * @return {S} + * + * @template S + */ +export const getAccessorValue = ( + accessor: AccessorNode +): S => + accessor.type === AST_NODE_TYPES.Identifier + ? accessor.name + : getStringValue(accessor); + +export type AccessorNode = + | StringNode + | KnownIdentifier; diff --git a/lib/node-utils/is-node-of-type.ts b/lib/node-utils/is-node-of-type.ts index afa2b3fc..12eeae38 100644 --- a/lib/node-utils/is-node-of-type.ts +++ b/lib/node-utils/is-node-of-type.ts @@ -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 ); diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index bfbbc346..0027fcd3 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -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(); export default createTestingLibraryRule({ name: RULE_NAME, @@ -65,20 +63,11 @@ export default createTestingLibraryRule({ ? 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; @@ -100,6 +89,60 @@ export default createTestingLibraryRule({ } 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, }; diff --git a/lib/utils/index.ts b/lib/utils/index.ts index 299d4c36..64adbc7b 100644 --- a/lib/utils/index.ts +++ b/lib/utils/index.ts @@ -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[], diff --git a/lib/utils/resolve-to-testing-library-fn.ts b/lib/utils/resolve-to-testing-library-fn.ts new file mode 100644 index 00000000..a10f555c --- /dev/null +++ b/lib/utils/resolve-to-testing-library-fn.ts @@ -0,0 +1,183 @@ +import { DefinitionType } from '@typescript-eslint/scope-manager'; +import { + AST_NODE_TYPES, + ASTUtils, + TSESLint, + TSESTree, +} from '@typescript-eslint/utils'; + +import { + isImportDefaultSpecifier, + isImportExpression, + isProperty, + isImportSpecifier, + isTSImportEqualsDeclaration, + isCallExpression, +} from '../node-utils'; +import { + AccessorNode, + getAccessorValue, + getStringValue, + isIdentifier, + isStringNode, + isSupportedAccessor, +} from '../node-utils/accessors'; + +import { LIBRARY_MODULES } from '.'; + +interface ImportDetails { + source: string; + local: string; + imported: string | null; +} + +const describeImportDefAsImport = ( + def: TSESLint.Scope.Definitions.ImportBindingDefinition +): ImportDetails | null => { + if (isTSImportEqualsDeclaration(def.parent)) { + return null; + } + + if (isImportDefaultSpecifier(def.node)) { + return { + source: def.parent.source.value, + imported: null, + local: def.node.local.name, + }; + } + + if (!isImportSpecifier(def.node)) { + return null; + } + + // we only care about value imports + if (def.parent.importKind === 'type') { + return null; + } + + return { + source: def.parent.source.value, + imported: + 'name' in def.node.imported + ? def.node.imported.name + : def.node.imported.value, + local: def.node.local.name, + }; +}; + +const describeVariableDefAsImport = ( + def: TSESLint.Scope.Definitions.VariableDefinition +): ImportDetails | null => { + if (!def.node.init) return null; + + const sourceNode = + isCallExpression(def.node.init) && + isIdentifier(def.node.init.callee, 'require') + ? def.node.init.arguments[0] + : ASTUtils.isAwaitExpression(def.node.init) && + isImportExpression(def.node.init.argument) + ? def.node.init.argument.source + : null; + + if (!sourceNode || !isStringNode(sourceNode)) return null; + if (!isProperty(def.name.parent)) return null; + if (!isSupportedAccessor(def.name.parent.key)) return null; + + return { + source: getStringValue(sourceNode), + imported: getAccessorValue(def.name.parent.key), + local: def.name.name, + }; +}; + +const describePossibleImportDef = ( + def: TSESLint.Scope.Definition +): ImportDetails | null => { + if (def.type === DefinitionType.Variable) { + return describeVariableDefAsImport(def); + } + if (def.type === DefinitionType.ImportBinding) { + return describeImportDefAsImport(def); + } + return null; +}; + +const resolveScope = ( + scope: TSESLint.Scope.Scope, + identifier: string +): ImportDetails | 'local' | null => { + let currentScope: TSESLint.Scope.Scope | null = scope; + while (currentScope !== null) { + const ref = currentScope.set.get(identifier); + if (ref && ref.defs.length > 0) { + const def = ref.defs[ref.defs.length - 1]; + const importDetails = describePossibleImportDef(def); + + if (importDetails?.local === identifier) { + return importDetails; + } + + return 'local'; + } + + currentScope = currentScope.upper; + } + + return null; +}; + +const joinChains = ( + a: AccessorNode[] | null, + b: AccessorNode[] | null +): AccessorNode[] | null => (a && b ? [...a, ...b] : null); + +export const getNodeChain = (node: TSESTree.Node): AccessorNode[] | null => { + if (isSupportedAccessor(node)) { + return [node]; + } + + switch (node.type) { + case AST_NODE_TYPES.MemberExpression: + return joinChains(getNodeChain(node.object), getNodeChain(node.property)); + case AST_NODE_TYPES.CallExpression: + return getNodeChain(node.callee); + } + + return null; +}; + +interface ResolvedTestingLibraryUserEventFn { + original: string | null; + local: string; +} + +const USER_EVENT_PACKAGE = '@testing-library/user-event'; + +export const resolveToTestingLibraryFn = ( + node: TSESTree.CallExpression, + context: TSESLint.RuleContext +): ResolvedTestingLibraryUserEventFn | null => { + const chain = getNodeChain(node); + if (!chain?.length) return null; + + const identifier = chain[0]; + const scope = context.sourceCode.getScope(identifier); + const maybeImport = resolveScope(scope, getAccessorValue(identifier)); + + if (maybeImport === 'local' || maybeImport === null) { + return null; + } + + if ( + [...LIBRARY_MODULES, USER_EVENT_PACKAGE].some( + (module) => module === maybeImport.source + ) + ) { + return { + original: maybeImport.imported, + local: maybeImport.local, + }; + } + + return null; +}; diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index 32960944..47ed6574 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -5,7 +5,7 @@ import rule, { Options, MessageIds, } from '../../../lib/rules/no-node-access'; -import { EVENT_HANDLER_METHODS, EVENTS_SIMULATORS } from '../../../lib/utils'; +import { EVENT_HANDLER_METHODS } from '../../../lib/utils'; import { createRuleTester } from '../test-utils'; const ruleTester = createRuleTester(); @@ -173,14 +173,52 @@ ruleTester.run(RULE_NAME, rule, { user.click(buttonText); `, }, - ...EVENTS_SIMULATORS.map((simulator) => ({ + { + code: ` + import userEvent from '@testing-library/user-event'; + import { screen } from '${testingFramework}'; + + const buttonText = screen.getByText('submit'); + const userAlias = userEvent.setup(); + userAlias.click(buttonText); + `, + }, + { code: ` + import userEvent from '@testing-library/user-event'; import { screen } from '${testingFramework}'; const buttonText = screen.getByText('submit'); - ${simulator}.click(buttonText); + userEvent.setup().click(buttonText); `, - })), + }, + { + code: ` + import userEvt from '@testing-library/user-event'; + import { screen } from '${testingFramework}'; + + const buttonText = screen.getByText('submit'); + const userAlias = userEvt.setup(); + userAlias.click(buttonText); + `, + }, + { + code: ` + import userEvt from '@testing-library/user-event'; + import { screen } from '${testingFramework}'; + + const buttonText = screen.getByText('submit'); + userEvt.click(buttonText); + `, + }, + { + code: ` + import { screen, fireEvent as fe } from '${testingFramework}'; + + const buttonText = screen.getByText('submit'); + fe.click(buttonText); + `, + }, ] ), invalid: SUPPORTED_TESTING_FRAMEWORKS.flatMap((testingFramework) => [ diff --git a/tests/lib/utils/resolve-to-testing-library-fn.test.ts b/tests/lib/utils/resolve-to-testing-library-fn.test.ts new file mode 100644 index 00000000..c306e568 --- /dev/null +++ b/tests/lib/utils/resolve-to-testing-library-fn.test.ts @@ -0,0 +1,234 @@ +import { InvalidTestCase } from '@typescript-eslint/rule-tester'; + +import { createTestingLibraryRule } from '../../../lib/create-testing-library-rule'; +import { LIBRARY_MODULES } from '../../../lib/utils'; +import { resolveToTestingLibraryFn } from '../../../lib/utils/resolve-to-testing-library-fn'; +import { createRuleTester } from '../test-utils'; + +type MessageIds = 'details'; + +const rule = createTestingLibraryRule<[], MessageIds>({ + name: __filename, + meta: { + docs: { + recommendedConfig: { + dom: 'error', + angular: 'error', + react: 'error', + vue: 'error', + svelte: 'error', + marko: 'error', + }, + description: 'Fake rule for testing parseUserEventFnCall', + }, + messages: { + details: '{{ data }}', + }, + schema: [], + type: 'problem', + }, + defaultOptions: [], + create: (context) => ({ + CallExpression(node) { + const testingLibraryFn = resolveToTestingLibraryFn(node, context); + + if (testingLibraryFn) { + context.report({ + messageId: 'details', + node, + data: { + data: testingLibraryFn, + }, + }); + } + }, + }), +}); + +const ruleTester = createRuleTester(); + +ruleTester.run('esm', rule, { + valid: [ + { + code: ` + import { userEvent } from './test-utils'; + + (userEvent => userEvent.setup)(); + `, + }, + { + code: ` + import { userEvent } from './test-utils'; + + function userClick() { + userEvent.click(document.body); + } + [].forEach(userClick); + `, + }, + { + code: ` + import { userEvent } from './test-utils'; + + userEvent.setup() + `, + }, + ...LIBRARY_MODULES.map((module) => ({ + code: ` + import * as testingLibrary from '${module}'; + + const { fireEvent } = testingLibrary + fireEvent.click(document.body) + `, + })), + ], + invalid: [ + { + code: ` + import userEvent from '@testing-library/user-event'; + + userEvent.setup() + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: null, + local: 'userEvent', + }, + }, + }, + ], + }, + ...LIBRARY_MODULES.flatMap>((module) => [ + { + code: ` + import { fireEvent } from '${module}'; + + fireEvent.click(document.body) + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: 'fireEvent', + local: 'fireEvent', + }, + }, + }, + ], + }, + { + code: ` + import { fireEvent as fe } from '${module}'; + + fe.click(document.body) + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: 'fireEvent', + local: 'fe', + }, + }, + }, + ], + }, + ]), + ], +}); + +ruleTester.run('cjs', rule, { + valid: [ + { + code: ` + const { userEvent } = require('./test-utils'); + + userEvent.setup() + `, + }, + ...LIBRARY_MODULES.map((module) => ({ + code: ` + const testingLibrary = require('${module}'); + + const { fireEvent } = testingLibrary + fireEvent.click(document.body) + `, + })), + ], + invalid: [ + { + code: ` + const { default: userEvent } = require('@testing-library/user-event'); + + userEvent.setup() + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: null, + local: 'userEvent', + }, + }, + }, + ], + }, + ...LIBRARY_MODULES.flatMap>((module) => [ + { + code: ` + const { fireEvent } = require('${module}'); + + fireEvent.click(document.body) + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: 'fireEvent', + local: 'fireEvent', + }, + }, + }, + ], + }, + { + code: ` + const { fireEvent: fe } = require('${module}'); + + fe.click(document.body) + `, + errors: [ + { + messageId: 'details', + data: { + data: { + original: 'fireEvent', + local: 'fe', + }, + }, + }, + ], + }, + ]), + ], +}); + +ruleTester.run('typescript', rule, { + valid: [ + { + code: ` + import userEvent = require('@testing-library/user-event'); + + userEvent.setup() + `, + }, + ], + invalid: [], +});