Skip to content

fix: consider promise.all() in await-async-utils #236

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 12, 2020
Merged

fix: consider promise.all() in await-async-utils #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions docs/rules/await-async-utils.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ test('something correctly', async () => {
// return the promise within a function is correct too!
const makeCustomWait = () =>
waitForElementToBeRemoved(() => document.querySelector('div.getOuttaHere'));

// using Promise.all combining the methods
await Promise.all([
waitFor(() => getByLabelText('email')),
waitForElementToBeRemoved(() => document.querySelector('div.getOuttaHere')),
]);
});
```

Expand Down
7 changes: 7 additions & 0 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,12 @@ module.exports = {
lines: 100,
statements: 100,
},
// TODO drop this custom threshold in v4
"./lib/node-utils.ts": {
branches: 90,
functions: 90,
lines: 90,
statements: 90,
}
},
};
12 changes: 12 additions & 0 deletions lib/node-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ export function isImportSpecifier(
return node && node.type === AST_NODE_TYPES.ImportSpecifier;
}

export function isImportNamespaceSpecifier(
node: TSESTree.Node
): node is TSESTree.ImportNamespaceSpecifier {
return node?.type === AST_NODE_TYPES.ImportNamespaceSpecifier
}

export function isImportDefaultSpecifier(
node: TSESTree.Node
): node is TSESTree.ImportDefaultSpecifier {
Expand Down Expand Up @@ -143,6 +149,12 @@ export function isReturnStatement(
return node && node.type === AST_NODE_TYPES.ReturnStatement;
}

export function isArrayExpression(
node: TSESTree.Node
): node is TSESTree.ArrayExpression {
return node?.type === AST_NODE_TYPES.ArrayExpression
}

export function isAwaited(node: TSESTree.Node) {
return (
isAwaitExpression(node) ||
Expand Down
26 changes: 22 additions & 4 deletions lib/rules/await-async-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import {
isAwaited,
isPromiseResolved,
getVariableReferences,
isMemberExpression,
isImportSpecifier,
isImportNamespaceSpecifier,
isCallExpression,
isArrayExpression,
isIdentifier
} from '../node-utils';

export const RULE_NAME = 'await-async-utils';
Expand All @@ -13,6 +19,17 @@ type Options = [];

const ASYNC_UTILS_REGEXP = new RegExp(`^(${ASYNC_UTILS.join('|')})$`);

// verifies the CallExpression is Promise.all()
function isPromiseAll(node: TSESTree.CallExpression) {
return isMemberExpression(node.callee) && isIdentifier(node.callee.object) && node.callee.object.name === 'Promise' && isIdentifier(node.callee.property) && node.callee.property.name === 'all'
}

// verifies the node is part of an array used in a CallExpression
function isInPromiseAll(node: TSESTree.Node) {
const parent = node.parent
return isCallExpression(parent) && isArrayExpression(parent.parent) && isCallExpression(parent.parent.parent) && isPromiseAll(parent.parent.parent)
}

Comment on lines +22 to +32
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if these are worth moving elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, they could be useful. We can move them when necessary tho.

export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
name: RULE_NAME,
meta: {
Expand Down Expand Up @@ -45,11 +62,11 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({

if (!LIBRARY_MODULES.includes(parent.source.value.toString())) return;

if (node.type === 'ImportSpecifier') {
if (isImportSpecifier(node)) {
importedAsyncUtils.push(node.imported.name);
}

if (node.type === 'ImportNamespaceSpecifier') {
if (isImportNamespaceSpecifier(node)) {
importedAsyncUtils.push(node.local.name);
}
},
Expand All @@ -72,7 +89,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
},
'Program:exit'() {
const testingLibraryUtilUsage = asyncUtilsUsage.filter(usage => {
if (usage.node.type === 'MemberExpression') {
if (isMemberExpression(usage.node)) {
const object = usage.node.object as TSESTree.Identifier;

return importedAsyncUtils.includes(object.name);
Expand All @@ -88,7 +105,8 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({
references &&
references.length === 0 &&
!isAwaited(node.parent.parent) &&
!isPromiseResolved(node)
!isPromiseResolved(node) &&
!isInPromiseAll(node)
) {
context.report({
node,
Expand Down
56 changes: 55 additions & 1 deletion tests/lib/rules/await-async-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,50 @@ ruleTester.run(RULE_NAME, rule, {
});
`,
})),

...ASYNC_UTILS.map(asyncUtil => ({
code: `
import { ${asyncUtil} } from '@testing-library/dom';
test('${asyncUtil} util used in with Promise.all() does not trigger an error', async () => {
await Promise.all([
${asyncUtil}(callback1),
${asyncUtil}(callback2),
]);
});
`,
})),
...ASYNC_UTILS.map(asyncUtil => ({
code: `
import { ${asyncUtil} } from '@testing-library/dom';
test('${asyncUtil} util used in with Promise.all() with an await does not trigger an error', async () => {
await Promise.all([
await ${asyncUtil}(callback1),
await ${asyncUtil}(callback2),
]);
});
`,
})),
...ASYNC_UTILS.map(asyncUtil => ({
code: `
import { ${asyncUtil} } from '@testing-library/dom';
test('${asyncUtil} util used in with Promise.all() with ".then" does not trigger an error', async () => {
Promise.all([
${asyncUtil}(callback1),
${asyncUtil}(callback2),
]).then(() => console.log('foo'));
});
`,
})),
{
code: `
import { waitFor, waitForElementToBeRemoved } from '@testing-library/dom';
test('combining different async methods with Promise.all does not throw an error', async () => {
await Promise.all([
waitFor(() => getByLabelText('email')),
waitForElementToBeRemoved(() => document.querySelector('div.getOuttaHere')),
])
});
`
},
{
code: `
import { waitForElementToBeRemoved } from '@testing-library/dom';
Expand All @@ -139,6 +182,17 @@ ruleTester.run(RULE_NAME, rule, {
});
`,
},
{
code: `
test('using unrelated promises with Promise.all do not throw an error', async () => {
await Promise.all([
someMethod(),
promise1,
await foo().then(() => baz())
])
})
`
}
],
invalid: [
...ASYNC_UTILS.map(asyncUtil => ({
Expand Down