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

feat(rule): add no-await-sync-events rule #240

Merged
merged 6 commits into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions docs/rules/no-await-sync-events.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# Disallow unnecessary `await` for sync events (no-await-sync-events)

Ensure that sync 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`. Some examples are:
Copy link
Member

Choose a reason for hiding this comment

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

This is technically true, but you don't need to always wait for it. From user-event doc:

To be clear, userEvent.type always returns a promise, but you only need to await the promise it returns if you're using the delay option. Otherwise everything runs synchronously and you can ignore the promise.

So should this rule report about userEvent.type if no delay option passed? I think so

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with that ☝️ we should only await if the delay option is used. It would be great if we can cover that scenario with the rule

Copy link
Contributor Author

@J-Huang J-Huang Oct 26, 2020

Choose a reason for hiding this comment

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

Agree. upgraded
the rule is, only if userEvent.type with delay option, await is valid. For all the other cases, it disallows await.


- `fireEvent.click`
- `fireEvent.select`
- `userEvent.tab`
- `userEvent.hover`

This rule aims to prevent users from waiting for those function calls.

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

```js
const foo = async () => {
// ...
await fireEvent.click(button);
// ...
};

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

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

```js
const foo = () => {
// ...
fireEvent.click(button);
// ...
};

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

## Notes

There is another rule `await-fire-event`, which is only in Vue Testing
Library. Please do not confuse with this rule.
2 changes: 2 additions & 0 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import awaitAsyncQuery from './rules/await-async-query';
import awaitAsyncUtils from './rules/await-async-utils';
import awaitFireEvent from './rules/await-fire-event';
import consistentDataTestid from './rules/consistent-data-testid';
import noAwaitSyncEvents from './rules/no-await-sync-events';
import noAwaitSyncQuery from './rules/no-await-sync-query';
import noDebug from './rules/no-debug';
import noDomImport from './rules/no-dom-import';
Expand All @@ -20,6 +21,7 @@ const rules = {
'await-async-utils': awaitAsyncUtils,
'await-fire-event': awaitFireEvent,
'consistent-data-testid': consistentDataTestid,
'no-await-sync-events': noAwaitSyncEvents,
'no-await-sync-query': noAwaitSyncQuery,
'no-debug': noDebug,
'no-dom-import': noDomImport,
Expand Down
50 changes: 50 additions & 0 deletions lib/rules/no-await-sync-events.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils';
import { getDocsUrl, ASYNC_EVENTS } from '../utils';

export const RULE_NAME = 'no-await-sync-events';
export type MessageIds = 'noAwaitSyncEvents';
type Options = [];

const ASYNC_EVENTS_REGEXP = new RegExp(`^(${ASYNC_EVENTS.join('|')})$`);
export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
name: RULE_NAME,
meta: {
type: 'problem',
docs: {
description: 'Disallow unnecessary `await` for sync events',
category: 'Best Practices',
recommended: 'error',
},
messages: {
noAwaitSyncEvents: '`{{ name }}` does not need `await` operator',
},
fixable: null,
schema: [],
},
defaultOptions: [],

create(context) {
// userEvent.type() is an exception, which returns a
// Promise, even it resolves immediately.
// for the sake of semantically correct, w/ or w/o await
// are both OK
return {
[`AwaitExpression > CallExpression > MemberExpression > Identifier[name=${ASYNC_EVENTS_REGEXP}]`](
node: TSESTree.Identifier
) {
const memberExpression = node.parent as TSESTree.MemberExpression;
const methodNode = memberExpression.property as TSESTree.Identifier;

if (!(node.name === 'userEvent' && methodNode.name === 'type')) {
context.report({
node: methodNode,
messageId: 'noAwaitSyncEvents',
data: {
name: `${node.name}.${methodNode.name}`,
},
});
}
},
};
},
});
6 changes: 6 additions & 0 deletions lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ const ASYNC_UTILS = [
'waitForDomChange',
];

const ASYNC_EVENTS = [
Copy link
Member

@Belco90 Belco90 Oct 23, 2020

Choose a reason for hiding this comment

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

These are SYNC_EVENTS actually, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't say there are events either - there are helpers or utilities.. Events would be "tab", "input", "change", "click" and so on... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initially thought it should be no-await-sync-utils, but feel not so correct...I can rename it to utils if it makes more sense.

'fireEvent',
'userEvent',
];

const TESTING_FRAMEWORK_SETUP_HOOKS = ['beforeEach', 'beforeAll'];

const PRESENCE_MATCHERS = ['toBeInTheDocument', 'toBeTruthy', 'toBeDefined'];
Expand All @@ -78,6 +83,7 @@ export {
ASYNC_QUERIES_COMBINATIONS,
ALL_QUERIES_COMBINATIONS,
ASYNC_UTILS,
ASYNC_EVENTS,
TESTING_FRAMEWORK_SETUP_HOOKS,
LIBRARY_MODULES,
PRESENCE_MATCHERS,
Expand Down
157 changes: 157 additions & 0 deletions tests/lib/rules/no-await-sync-events.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
import { createRuleTester } from '../test-utils';
import rule, { RULE_NAME } from '../../../lib/rules/no-await-sync-events';
import { ASYNC_EVENTS } from '../../../lib/utils';

const ruleTester = createRuleTester();

const fireEventFunctions = [
'copy',
'cut',
'paste',
'compositionEnd',
'compositionStart',
'compositionUpdate',
'keyDown',
'keyPress',
'keyUp',
'focus',
'blur',
'focusIn',
'focusOut',
'change',
'input',
'invalid',
'submit',
'reset',
'click',
'contextMenu',
'dblClick',
'drag',
'dragEnd',
'dragEnter',
'dragExit',
'dragLeave',
'dragOver',
'dragStart',
'drop',
'mouseDown',
'mouseEnter',
'mouseLeave',
'mouseMove',
'mouseOut',
'mouseOver',
'mouseUp',
'popState',
'select',
'touchCancel',
'touchEnd',
'touchMove',
'touchStart',
'scroll',
'wheel',
'abort',
'canPlay',
'canPlayThrough',
'durationChange',
'emptied',
'encrypted',
'ended',
'loadedData',
'loadedMetadata',
'loadStart',
'pause',
'play',
'playing',
'progress',
'rateChange',
'seeked',
'seeking',
'stalled',
'suspend',
'timeUpdate',
'volumeChange',
'waiting',
'load',
'error',
'animationStart',
'animationEnd',
'animationIteration',
'transitionEnd',
'doubleClick',
'pointerOver',
'pointerEnter',
'pointerDown',
'pointerMove',
'pointerUp',
'pointerCancel',
'pointerOut',
'pointerLeave',
'gotPointerCapture',
'lostPointerCapture',
];
const userEventFunctions = [
'clear',
'click',
'dblClick',
'selectOptions',
'deselectOptions',
'upload',
// 'type',
'tab',
'paste',
'hover',
'unhover',
];
let eventFunctions: string[] = [];
ASYNC_EVENTS.forEach(event => {
switch (event) {
case 'fireEvent':
eventFunctions = eventFunctions.concat(fireEventFunctions.map((f: string): string => `${event}.${f}`));
break;
case 'userEvent':
eventFunctions = eventFunctions.concat(userEventFunctions.map((f: string): string => `${event}.${f}`));
break;
default:
eventFunctions.push(`${event}.anyFunc`);
}
});

ruleTester.run(RULE_NAME, rule, {
valid: [
Copy link
Member

Choose a reason for hiding this comment

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

You need to include more tests for checking userEvent and fireEvent non-related to Testing Library are not reported so they are valid with await operator.

There is an ongoing refactor for v4 of the plugin which actually will check that for all rules out of the box, so I don't know if you prefer to wait for that version to be released.

Copy link
Contributor Author

@J-Huang J-Huang Oct 26, 2020

Choose a reason for hiding this comment

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

@Belco90
Do you mean how about if userEvent or fireEvent are user named objects, it should not apply the rule? Or, if extra functions are added to those objects, for those cases, await may be valid?
In order to ensure that, it has to check if the property name in userEvent or fireEvent is one of the expected event names, such as click, type, tab and so on. Even those methods can be overridden. Am I misunderstanding the question?

Copy link
Member

Choose a reason for hiding this comment

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

The former, so we don't report const declared by the user with the same name. As mentioned, the internal refactor for v4 will pass some helpers to all rules to check this generically, so I'm not sure if you prefer to wait for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification.
will refactor the code by then if needed.

// sync events without await are valid
// userEvent.type() is an exception
...eventFunctions.map(func => ({
code: `() => {
${func}('foo')
}
`,
})),
{
code: `() => {
userEvent.type('foo')
}
`,
},
{
code: `() => {
await userEvent.type('foo')
}
`,
},
],

invalid: [
// sync events with await operator are not valid
...eventFunctions.map(func => ({
code: `() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you write more realistic code here? So you end up with something like:

import { fireEvent } from '@testing-library/framework'
import userEvent from '@testing-library/user-event'
import MyComponent from './MyComponent'

it('should report sync event awaited', async () => {
  render(<MyComponent />)
  
  await ${func}('foo')
  await findByQuery('some element')
})

and then check in the errors which line the error should appear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

await ${func}('foo')
}
`,
errors: [
{
messageId: 'noAwaitSyncEvents',
},
],
})),
],
});