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-wait-for-side-effects rule #300

Merged
merged 10 commits into from
Mar 27, 2021
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@
[![Tweet][tweet-badge]][tweet-url]

<!-- ALL-CONTRIBUTORS-BADGE:START - Do not remove or modify this section -->

[![All Contributors](https://img.shields.io/badge/all_contributors-36-orange.svg?style=flat-square)](#contributors-)

<!-- ALL-CONTRIBUTORS-BADGE:END -->

## Installation
Expand Down Expand Up @@ -139,9 +141,8 @@ To enable this configuration use the `extends` property in your
| [no-node-access](docs/rules/no-node-access.md) | Disallow direct Node access | ![angular-badge][] ![react-badge][] ![vue-badge][] | |
| [no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | | |
| [no-render-in-setup](docs/rules/no-render-in-setup.md) | Disallow the use of `render` in setup functions | | |
| [no-side-effects-wait-for](docs/rules/no-side-effects-wait-for.md) | Disallow the use of side effects inside `waitFor` | | |
| [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | |
| [no-wait-for-snapshot](docs/rules/no-wait-for-snapshot.md) | Ensures no snapshot is generated inside of a `waitFor` call | | |
| [no-wait-for-side-effects](docs/rules/no-wait-for-side-effects.md) | Disallow the use of side effects inside `waitFor` | | |
| [no-wait-for-snapshot](docs/rules/no-wait-for-snapshot.md) | Ensures no snapshot is generated inside of a `waitFor` call | | |
| [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | |
| [prefer-find-by](docs/rules/prefer-find-by.md) | Suggest using `findBy*` methods instead of the `waitFor` + `getBy` queries | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] |
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Side effects inside `waitFor` are not preferred (no-side-effects-wait-for)
# Side effects inside `waitFor` are not preferred (no-wait-for-side-effects)

## Rule Details

Expand Down
143 changes: 96 additions & 47 deletions lib/detect-testing-library-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ export interface DetectionHelpers {
isQuery: IsQueryFn;
isCustomQuery: IsCustomQueryFn;
isAsyncUtil: IsAsyncUtilFn;
isFireEventUtil: (node: TSESTree.Identifier) => boolean;
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 defined types inline here instead of extracting them. I'm not sure what was the advantage of that (not having to write it twice?) but both ways are pretty awful. I may refactor all types here to inline types.

isUserEventUtil: (node: TSESTree.Identifier) => boolean;
isFireEventMethod: IsFireEventMethodFn;
isRenderUtil: IsRenderUtilFn;
isRenderVariableDeclarator: IsRenderVariableDeclaratorFn;
Expand All @@ -107,7 +109,6 @@ export interface DetectionHelpers {
isNodeComingFromTestingLibrary: IsNodeComingFromTestingLibraryFn;
}

const FIRE_EVENT_NAME = 'fireEvent';
const RENDER_NAME = 'render';

/**
Expand Down Expand Up @@ -173,6 +174,69 @@ 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.

Extracting generic logic from isFireEventMethod so I can reuse it for isUserEventMethod, which hasn't been implemented here yet but will be in the next PR. I thought I'd need it here but then I realized I wanted isUserEventUtil instead of isUserEventMethod.

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) ||
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'm pretty sure this could be simplified by using isTestingLibraryUtil here but I don't want to start this refactor since it's working fine. I'll leave it for a future nice to have.

(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 @@ -308,55 +372,38 @@ export function detectTestingLibraryUtils<
};

/**
* Determines whether a given node is fireEvent method or not
* Determines whether a given node is fireEvent util itself or not.
*
* Not to be confused with {@link isFireEventMethod}
*/
const isFireEventMethod: IsFireEventMethodFn = (node) => {
const fireEventUtil = findImportedUtilSpecifier(FIRE_EVENT_NAME);
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;
const isFireEventUtil = (node: TSESTree.Identifier): boolean => {
return isTestingLibraryUtil(
node,
(identifierNodeName, originalNodeName) => {
return [identifierNodeName, originalNodeName].includes('fireEvent');
}
);
};

// 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;
/**
* Determines whether a given node is userEvent util itself or not.
*
* Not to be confused with {@link isUserEventMethod}
Copy link
Member Author

Choose a reason for hiding this comment

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

isUserEventMethod doesn't exist yet, but as I said it's coming in the next PR.

*/
const isUserEventUtil = (node: TSESTree.Identifier): boolean => {
return isTestingLibraryUtil(
node,
(identifierNodeName, originalNodeName) => {
return [identifierNodeName, originalNodeName].includes('userEvent');
}
);
};

return regularCall || wildcardCall;
/**
* Determines whether a given node is fireEvent method or not
*/
const isFireEventMethod: IsFireEventMethodFn = (node) => {
return isTestingLibrarySimulateEventUtil(node, 'fireEvent');
};

/**
Expand Down Expand Up @@ -557,6 +604,8 @@ export function detectTestingLibraryUtils<
isQuery,
isCustomQuery,
isAsyncUtil,
isFireEventUtil,
isUserEventUtil,
isFireEventMethod,
isRenderUtil,
isRenderVariableDeclarator,
Expand Down
4 changes: 2 additions & 2 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import preferUserEvent from './rules/prefer-user-event';
import preferWaitFor from './rules/prefer-wait-for';
import noMultipleAssertionsWaitFor from './rules/no-multiple-assertions-wait-for';
import preferFindBy from './rules/prefer-find-by';
import noSideEffectsWaitFor from './rules/no-side-effects-wait-for';
import noWaitForSideEffects from './rules/no-wait-for-side-effects';
import renderResultNamingConvention from './rules/render-result-naming-convention';

const rules = {
Expand All @@ -38,8 +38,8 @@ const rules = {
'no-node-access': noNodeAccess,
'no-promise-in-fire-event': noPromiseInFireEvent,
'no-render-in-setup': noRenderInSetup,
'no-side-effects-wait-for': noSideEffectsWaitFor,
'no-wait-for-empty-callback': noWaitForEmptyCallback,
'no-wait-for-side-effects': noWaitForSideEffects,
'no-wait-for-snapshot': noWaitForSnapshot,
'prefer-explicit-assert': preferExplicitAssert,
'prefer-find-by': preferFindBy,
Expand Down
10 changes: 10 additions & 0 deletions lib/node-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ export function isJSXAttribute(
return node?.type === AST_NODE_TYPES.JSXAttribute;
}

export function isExpressionStatement(
node: TSESTree.Node
): node is TSESTree.ExpressionStatement {
return node?.type === AST_NODE_TYPES.ExpressionStatement;
}

/**
* Finds the closest CallExpression node for a given node.
* @param node
Expand Down Expand Up @@ -388,6 +394,10 @@ export function getPropertyIdentifierNode(
return getPropertyIdentifierNode(node.callee);
}

if (isExpressionStatement(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.

Improving node checks here.

return getPropertyIdentifierNode(node.expression);
}

return null;
}

Expand Down
78 changes: 0 additions & 78 deletions lib/rules/no-side-effects-wait-for.ts

This file was deleted.