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-promise-in-fire-event #266

Merged
merged 8 commits into from
Dec 7, 2020
28 changes: 20 additions & 8 deletions docs/rules/no-promise-in-fire-event.md
Original file line number Diff line number Diff line change
@@ -1,33 +1,45 @@
# Disallow the use of promises passed to a `fireEvent` method (no-promise-in-fire-event)

The `fireEvent` method expects that a DOM element is passed.
Methods from `fireEvent` expect to receive a DOM element. Passing a promise will end up in an error, so it must be prevented.

Examples of **incorrect** code for this rule:

```js
import { screen, fireEvent } from '@testing-library/react';

// usage of findBy queries
// usage of unhandled findBy queries
fireEvent.click(screen.findByRole('button'));

// usage of promises
fireEvent.click(new Promise(jest.fn())
// usage of unhandled promises
fireEvent.click(new Promise(jest.fn()));

// usage of references to unhandled promises
const promise = new Promise();
fireEvent.click(promise);

const anotherPromise = screen.findByRole('button');
fireEvent.click(anotherPromise);
```

Examples of **correct** code for this rule:

```js
import { screen, fireEvent } from '@testing-library/react';

// use getBy queries
// usage of getBy queries
fireEvent.click(screen.getByRole('button'));

// use awaited findBy queries
// usage of awaited findBy queries
fireEvent.click(await screen.findByRole('button'));

// this won't give a linting error, but it will throw a runtime error
// usage of references to handled promises
const promise = new Promise();
fireEvent.click(promise)`,
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 one is reported properly now! It has been moved to "incorrect" section.

const element = await promise;
fireEvent.click(element);

const anotherPromise = screen.findByRole('button');
const button = await anotherPromise;
fireEvent.click(button);
```

## Further Reading
Expand Down
28 changes: 9 additions & 19 deletions lib/node-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,6 @@ export function isBlockStatement(
return node?.type === AST_NODE_TYPES.BlockStatement;
}

export function isVariableDeclarator(
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this one in favor of ASTUtils one

node: TSESTree.Node
): node is TSESTree.VariableDeclarator {
return node?.type === AST_NODE_TYPES.VariableDeclarator;
}

export function isObjectPattern(
node: TSESTree.Node
): node is TSESTree.ObjectPattern {
Expand Down Expand Up @@ -191,14 +185,6 @@ export function isImportDeclaration(
return node?.type === AST_NODE_TYPES.ImportDeclaration;
}

export function isAwaited(node: TSESTree.Node): 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.

Not used anymore

return (
ASTUtils.isAwaitExpression(node) ||
isArrowFunctionExpression(node) ||
isReturnStatement(node)
);
}

export function hasChainedThen(node: TSESTree.Node): boolean {
const parent = node.parent;

Expand All @@ -211,11 +197,16 @@ export function hasChainedThen(node: TSESTree.Node): boolean {
return hasThenProperty(parent);
}

export function isPromiseIdentifier(
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this one 👍

Copy link
Member Author

@Belco90 Belco90 Dec 7, 2020

Choose a reason for hiding this comment

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

I got the inspiration for typing it from ts-eslint itself, it's a really cool way of checking specific node types with a particular attribute.

node: TSESTree.Node
): node is TSESTree.Identifier & { name: 'Promise' } {
return ASTUtils.isIdentifier(node) && node.name === 'Promise';
}

export function isPromiseAll(node: TSESTree.CallExpression): boolean {
return (
isMemberExpression(node.callee) &&
ASTUtils.isIdentifier(node.callee.object) &&
node.callee.object.name === 'Promise' &&
isPromiseIdentifier(node.callee.object) &&
ASTUtils.isIdentifier(node.callee.property) &&
node.callee.property.name === 'all'
);
Expand All @@ -224,8 +215,7 @@ export function isPromiseAll(node: TSESTree.CallExpression): boolean {
export function isPromiseAllSettled(node: TSESTree.CallExpression): boolean {
return (
isMemberExpression(node.callee) &&
ASTUtils.isIdentifier(node.callee.object) &&
node.callee.object.name === 'Promise' &&
isPromiseIdentifier(node.callee.object) &&
ASTUtils.isIdentifier(node.callee.property) &&
node.callee.property.name === 'allSettled'
);
Expand Down Expand Up @@ -304,7 +294,7 @@ export function getVariableReferences(
node: TSESTree.Node
): TSESLint.Scope.Reference[] {
return (
(isVariableDeclarator(node) &&
(ASTUtils.isVariableDeclarator(node) &&
context.getDeclaredVariables(node)[0]?.references?.slice(1)) ||
[]
);
Expand Down
111 changes: 67 additions & 44 deletions lib/rules/no-promise-in-fire-event.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { createTestingLibraryRule } from '../create-testing-library-rule';
import {
TSESTree,
ESLintUtils,
ASTUtils,
} from '@typescript-eslint/experimental-utils';
import { getDocsUrl, ASYNC_QUERIES_VARIANTS } from '../utils';
import {
isNewExpression,
isImportSpecifier,
findClosestCallExpressionNode,
getIdentifierNode,
isCallExpression,
isNewExpression,
isPromiseIdentifier,
} from '../node-utils';

export const RULE_NAME = 'no-promise-in-fire-event';
export type MessageIds = 'noPromiseInFireEvent';
type Options = [];

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
export default createTestingLibraryRule<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
Expand All @@ -28,49 +26,74 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
noPromiseInFireEvent:
"A promise shouldn't be passed to a `fireEvent` method, instead pass the DOM element",
},
fixable: 'code',
fixable: 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.

This was set incorrectly.

schema: [],
},
defaultOptions: [],

create(context) {
return {
'ImportDeclaration[source.value=/testing-library/]'(
node: TSESTree.ImportDeclaration
) {
const fireEventImportNode = node.specifiers.find(
(specifier) =>
isImportSpecifier(specifier) &&
specifier.imported &&
'fireEvent' === specifier.imported.name
) as TSESTree.ImportSpecifier;
create(context, _, helpers) {
function checkSuspiciousNode(
node: TSESTree.Node,
originalNode?: TSESTree.Node
): void {
if (ASTUtils.isAwaitExpression(node)) {
return;
}

const { references } = context.getDeclaredVariables(
fireEventImportNode
)[0];
if (isNewExpression(node)) {
if (isPromiseIdentifier(node.callee)) {
return context.report({
node: originalNode ?? node,
messageId: 'noPromiseInFireEvent',
});
}
}

for (const reference of references) {
const referenceNode = reference.identifier;
const callExpression = referenceNode.parent
.parent as TSESTree.CallExpression;
const [element] = callExpression.arguments as TSESTree.Node[];
if (isCallExpression(element) || isNewExpression(element)) {
const methodName = ASTUtils.isIdentifier(element.callee)
? element.callee.name
: ((element.callee as TSESTree.MemberExpression)
.property as TSESTree.Identifier).name;
if (isCallExpression(node)) {
const domElementIdentifier = getIdentifierNode(node);

if (
helpers.isAsyncQuery(domElementIdentifier) ||
isPromiseIdentifier(domElementIdentifier)
) {
return context.report({
node: originalNode ?? node,
messageId: 'noPromiseInFireEvent',
});
}
}

if (ASTUtils.isIdentifier(node)) {
const nodeVariable = ASTUtils.findVariable(
context.getScope(),
node.name
);
if (!nodeVariable || !nodeVariable.defs) {
return;
}

if (
ASYNC_QUERIES_VARIANTS.some((q) => methodName.startsWith(q)) ||
methodName === 'Promise'
) {
context.report({
node: element,
messageId: 'noPromiseInFireEvent',
});
}
}
for (const definition of nodeVariable.defs) {
const variableDeclarator = definition.node as TSESTree.VariableDeclarator;
checkSuspiciousNode(variableDeclarator.init, node);
}
}
}

return {
'CallExpression Identifier'(node: TSESTree.Identifier) {
if (!helpers.isFireEventMethod(node)) {
return;
}

const closestCallExpression = findClosestCallExpressionNode(node, true);

if (!closestCallExpression) {
return;
}

const domElementArgument = closestCallExpression.arguments[0];

checkSuspiciousNode(domElementArgument);
},
};
},
Expand Down