Skip to content
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

Move rules settings to ESLint shared config: refactor no-await-sync-events rule #302

Merged
merged 10 commits into from
Mar 28, 2021
24 changes: 15 additions & 9 deletions docs/rules/no-await-sync-events.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
# Disallow unnecessary `await` for sync events (no-await-sync-events)

Ensure that sync events are not awaited unnecessarily.
Ensure that sync simulated events are not awaited unnecessarily.

## Rule Details

Functions in the event object provided by Testing Library, including
fireEvent and userEvent, do NOT return Promise, with an exception of
`userEvent.type`, which delays the promise resolve only if [`delay`
Methods for simulating events in Testing Library ecosystem -`fireEvent` and `userEvent`-
do NOT return any Promise, with an exception of
`userEvent.type` and `userEvent.keyboard`, which delays the promise resolve only if [`delay`
option](https://github.com/testing-library/user-event#typeelement-text-options) is specified.
Some examples are:

Some examples of simulating events not returning any Promise are:

- `fireEvent.click`
- `fireEvent.select`
Expand All @@ -26,15 +27,16 @@ const foo = async () => {
// ...
};

const bar = () => {
const bar = async () => {
// ...
await userEvent.tab();
// ...
};

const baz = () => {
const baz = async () => {
// ...
await userEvent.type(textInput, 'abc');
await userEvent.keyboard('abc');
// ...
};
```
Expand All @@ -54,10 +56,14 @@ const bar = () => {
// ...
};

const baz = () => {
const baz = async () => {
// await userEvent.type only with delay option
await userEvent.type(textInput, 'abc', {delay: 1000});
await userEvent.type(textInput, 'abc', { delay: 1000 });
userEvent.type(textInput, '123');

// same for userEvent.keyboard
await userEvent.keyboard(textInput, 'abc', { delay: 1000 });
userEvent.keyboard('123');
// ...
};
```
Expand Down
200 changes: 135 additions & 65 deletions lib/detect-testing-library-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
hasImportMatch,
ImportModuleNode,
isImportDeclaration,
isImportDefaultSpecifier,
isImportNamespaceSpecifier,
isImportSpecifier,
isLiteral,
Expand All @@ -20,9 +21,9 @@ import {
} from './node-utils';
import {
ABSENCE_MATCHERS,
ALL_QUERIES_COMBINATIONS,
ASYNC_UTILS,
PRESENCE_MATCHERS,
ALL_QUERIES_COMBINATIONS,
} from './utils';

export type TestingLibrarySettings = {
Expand Down Expand Up @@ -67,6 +68,7 @@ type IsAsyncUtilFn = (
validNames?: readonly typeof ASYNC_UTILS[number][]
) => boolean;
type IsFireEventMethodFn = (node: TSESTree.Identifier) => boolean;
type IsUserEventMethodFn = (node: TSESTree.Identifier) => boolean;
type IsRenderUtilFn = (node: TSESTree.Identifier) => boolean;
type IsRenderVariableDeclaratorFn = (
node: TSESTree.VariableDeclarator
Expand Down Expand Up @@ -99,6 +101,7 @@ export interface DetectionHelpers {
isFireEventUtil: (node: TSESTree.Identifier) => boolean;
isUserEventUtil: (node: TSESTree.Identifier) => boolean;
isFireEventMethod: IsFireEventMethodFn;
isUserEventMethod: IsUserEventMethodFn;
isRenderUtil: IsRenderUtilFn;
isRenderVariableDeclarator: IsRenderVariableDeclaratorFn;
isDebugUtil: IsDebugUtilFn;
Expand All @@ -109,6 +112,9 @@ export interface DetectionHelpers {
isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn;
}

const USER_EVENT_PACKAGE = '@testing-library/user-event';
const FIRE_EVENT_NAME = 'fireEvent';
const USER_EVENT_NAME = 'userEvent';
const RENDER_NAME = 'render';

/**
Expand All @@ -125,6 +131,7 @@ export function detectTestingLibraryUtils<
): TSESLint.RuleListener => {
let importedTestingLibraryNode: ImportModuleNode | null = null;
let importedCustomModuleNode: ImportModuleNode | null = null;
let importedUserEventLibraryNode: ImportModuleNode | null = null;

// Init options based on shared ESLint settings
const customModule = context.settings['testing-library/utils-module'];
Expand Down Expand Up @@ -174,69 +181,6 @@ export function detectTestingLibraryUtils<
return isNodeComingFromTestingLibrary(referenceNodeIdentifier);
}

/**
* Determines whether a given node is a simulate event util related to
* Testing Library or not.
*
* In order to determine this, the node must match:
* - indicated simulate event name: fireEvent or userEvent
* - imported from valid Testing Library module (depends on Aggressive
* Reporting)
*
*/
function isTestingLibrarySimulateEventUtil(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert this one since it can't be reused for user-event: this is a default import coming from a different module.

node: TSESTree.Identifier,
utilName: 'fireEvent' | 'userEvent'
): boolean {
const simulateEventUtil = findImportedUtilSpecifier(utilName);
let simulateEventUtilName: string | undefined;

if (simulateEventUtil) {
simulateEventUtilName = ASTUtils.isIdentifier(simulateEventUtil)
? simulateEventUtil.name
: simulateEventUtil.local.name;
} else if (isAggressiveModuleReportingEnabled()) {
simulateEventUtilName = utilName;
}

if (!simulateEventUtilName) {
return false;
}

const parentMemberExpression:
| TSESTree.MemberExpression
| undefined = isMemberExpression(node.parent) ? node.parent : undefined;

if (!parentMemberExpression) {
return false;
}

// make sure that given node it's not fireEvent/userEvent object itself
if (
[simulateEventUtilName, utilName].includes(node.name) ||
(ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === node.name)
) {
return false;
}

// check fireEvent.click()/userEvent.click() usage
const regularCall =
ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === simulateEventUtilName;

// check testingLibraryUtils.fireEvent.click() or
// testingLibraryUtils.userEvent.click() usage
const wildcardCall =
isMemberExpression(parentMemberExpression.object) &&
ASTUtils.isIdentifier(parentMemberExpression.object.object) &&
parentMemberExpression.object.object.name === simulateEventUtilName &&
ASTUtils.isIdentifier(parentMemberExpression.object.property) &&
parentMemberExpression.object.property.name === utilName;

return regularCall || wildcardCall;
}

/**
* Determines whether aggressive module reporting is enabled or not.
*
Expand Down Expand Up @@ -403,7 +347,90 @@ export function detectTestingLibraryUtils<
* Determines whether a given node is fireEvent method or not
*/
const isFireEventMethod: IsFireEventMethodFn = (node) => {
return isTestingLibrarySimulateEventUtil(node, 'fireEvent');
const fireEventUtil = findImportedUtilSpecifier(FIRE_EVENT_NAME);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just reverting isFireEventMethod to its original implementation.

let fireEventUtilName: string | undefined;

if (fireEventUtil) {
fireEventUtilName = ASTUtils.isIdentifier(fireEventUtil)
? fireEventUtil.name
: fireEventUtil.local.name;
} else if (isAggressiveModuleReportingEnabled()) {
fireEventUtilName = FIRE_EVENT_NAME;
}

if (!fireEventUtilName) {
return false;
}

const parentMemberExpression:
| TSESTree.MemberExpression
| undefined = isMemberExpression(node.parent) ? node.parent : undefined;

if (!parentMemberExpression) {
return false;
}

// make sure that given node it's not fireEvent object itself
if (
[fireEventUtilName, FIRE_EVENT_NAME].includes(node.name) ||
(ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === node.name)
) {
return false;
}

// check fireEvent.click() usage
const regularCall =
ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === fireEventUtilName;

// check testingLibraryUtils.fireEvent.click() usage
const wildcardCall =
isMemberExpression(parentMemberExpression.object) &&
ASTUtils.isIdentifier(parentMemberExpression.object.object) &&
parentMemberExpression.object.object.name === fireEventUtilName &&
ASTUtils.isIdentifier(parentMemberExpression.object.property) &&
parentMemberExpression.object.property.name === FIRE_EVENT_NAME;

return regularCall || wildcardCall;
};

const isUserEventMethod: IsUserEventMethodFn = (node) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New helper for detecting userEvent methods properly.

const userEvent = findImportedUserEventSpecifier();
let userEventName: string | undefined;

if (userEvent) {
userEventName = userEvent.name;
} else if (isAggressiveModuleReportingEnabled()) {
userEventName = USER_EVENT_NAME;
}

if (!userEventName) {
return false;
}

const parentMemberExpression:
| TSESTree.MemberExpression
| undefined = isMemberExpression(node.parent) ? node.parent : undefined;

if (!parentMemberExpression) {
return false;
}

// make sure that given node it's not userEvent object itself
if (
[userEventName, USER_EVENT_NAME].includes(node.name) ||
(ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === node.name)
) {
return false;
}

// check userEvent.click() usage
return (
ASTUtils.isIdentifier(parentMemberExpression.object) &&
parentMemberExpression.object.name === userEventName
);
};

/**
Expand Down Expand Up @@ -553,6 +580,27 @@ export function detectTestingLibraryUtils<
}
};

const findImportedUserEventSpecifier: () => TSESTree.Identifier | null = () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small function for finding imported specifier related to user-event

if (!importedUserEventLibraryNode) {
return null;
}

if (isImportDeclaration(importedUserEventLibraryNode)) {
const userEventIdentifier = importedUserEventLibraryNode.specifiers.find(
(specifier) => isImportDefaultSpecifier(specifier)
);

if (userEventIdentifier) {
return userEventIdentifier.local;
}
} else {
const requireNode = importedUserEventLibraryNode.parent as TSESTree.VariableDeclarator;
return requireNode.id as TSESTree.Identifier;
}

return null;
};

const getImportedUtilSpecifier = (
node: TSESTree.MemberExpression | TSESTree.Identifier
): TSESTree.ImportClause | TSESTree.Identifier | undefined => {
Expand Down Expand Up @@ -607,6 +655,7 @@ export function detectTestingLibraryUtils<
isFireEventUtil,
isUserEventUtil,
isFireEventMethod,
isUserEventMethod,
isRenderUtil,
isRenderVariableDeclarator,
isDebugUtil,
Expand Down Expand Up @@ -644,6 +693,15 @@ export function detectTestingLibraryUtils<
) {
importedCustomModuleNode = node;
}

// check only if user-event import not found yet so we avoid
// to override importedUserEventLibraryNode after it's found
if (
!importedUserEventLibraryNode &&
String(node.source.value) === USER_EVENT_PACKAGE
) {
importedUserEventLibraryNode = node;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since userEvent is imported from another package, I add to include its detection separately from the main Testing Library imported module.

}
},

// Check if Testing Library related modules are loaded with required.
Expand Down Expand Up @@ -676,6 +734,18 @@ export function detectTestingLibraryUtils<
) {
importedCustomModuleNode = callExpression;
}

if (
!importedCustomModuleNode &&
args.some(
(arg) =>
isLiteral(arg) &&
typeof arg.value === 'string' &&
arg.value === USER_EVENT_PACKAGE
)
) {
importedUserEventLibraryNode = callExpression;
}
},
};

Expand Down
Loading