Skip to content

Commit b121bd9

Browse files
kirkwaiblingerJosh-Cena
kirkwaiblinger
and
Josh-Cena
authored
feat(eslint-plugin): [return-await] check for-await loop iteree (#10008)
* check for-await loop iteree * test fixup * better docs and stuff * tweak * change comment * await=true Co-authored-by: Joshua Chen <sidachen2003@gmail.com> * type and remove unnecessary condition * Use Josh Cena's writeup instead Co-authored-by: Joshua Chen <sidachen2003@gmail.com> * snapshot * cov --------- Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
1 parent 16f4d90 commit b121bd9

4 files changed

Lines changed: 278 additions & 4 deletions

File tree

‎packages/eslint-plugin/docs/rules/await-thenable.mdx‎

Lines changed: 82 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import TabItem from '@theme/TabItem';
1010
> See **https://typescript-eslint.io/rules/await-thenable** for documentation.
1111
1212
A "Thenable" value is an object which has a `then` method, such as a Promise.
13-
The `await` keyword is generally used to retrieve the result of calling a Thenable's `then` method.
13+
The [`await` keyword](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await) is generally used to retrieve the result of calling a Thenable's `then` method.
1414

15-
If the `await` keyword is used on a value that is not a Thenable, the value is directly resolved immediately.
15+
If the `await` keyword is used on a value that is not a Thenable, the value is directly resolved, but will still pause execution until the next microtask.
1616
While doing so is valid JavaScript, it is often a programmer error, such as forgetting to add parenthesis to call a function that returns a Promise.
1717

1818
## Examples
@@ -40,6 +40,86 @@ await createValue();
4040
</TabItem>
4141
</Tabs>
4242

43+
## Async Iteration (`for await...of` Loops)
44+
45+
This rule also inspects [`for await...of` statements](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for-await...of), and reports if the value being iterated over is not async-iterable.
46+
47+
:::info[Why does the rule report on `for await...of` loops used on an array of Promises?]
48+
49+
While `for await...of` can be used with synchronous iterables, and it will await each promise produced by the iterable, it is inadvisable to do so.
50+
There are some tiny nuances that you may want to consider.
51+
52+
The biggest difference between using `for await...of` and using `for...of` (plus awaiting each result yourself) is error handling.
53+
When an error occurs within the loop body, `for await...of` does _not_ close the original sync iterable, while `for...of` does.
54+
For detailed examples of this, see the [MDN documentation on using `for await...of` with sync-iterables](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for-await...of#iterating_over_sync_iterables_and_generators).
55+
56+
Also consider whether you need sequential awaiting at all. Using `for await...of` may obscure potential opportunities for concurrent processing, such as those reported by [`no-await-in-loop`](https://eslint.org/docs/latest/rules/no-await-in-loop). Consider instead using one of the [promise concurrency methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#promise_concurrency) for better performance.
57+
58+
:::
59+
60+
### Examples
61+
62+
<Tabs>
63+
<TabItem value="❌ Incorrect">
64+
65+
```ts
66+
async function syncIterable() {
67+
const arrayOfValues = [1, 2, 3];
68+
for await (const value of arrayOfValues) {
69+
console.log(value);
70+
}
71+
}
72+
73+
async function syncIterableOfPromises() {
74+
const arrayOfPromises = [
75+
Promise.resolve(1),
76+
Promise.resolve(2),
77+
Promise.resolve(3),
78+
];
79+
for await (const promisedValue of arrayOfPromises) {
80+
console.log(promisedValue);
81+
}
82+
}
83+
```
84+
85+
</TabItem>
86+
<TabItem value="✅ Correct">
87+
88+
```ts
89+
async function syncIterable() {
90+
const arrayOfValues = [1, 2, 3];
91+
for (const value of arrayOfValues) {
92+
console.log(value);
93+
}
94+
}
95+
96+
async function syncIterableOfPromises() {
97+
const arrayOfPromises = [
98+
Promise.resolve(1),
99+
Promise.resolve(2),
100+
Promise.resolve(3),
101+
];
102+
for (const promisedValue of await Promise.all(arrayOfPromises)) {
103+
console.log(promisedValue);
104+
}
105+
}
106+
107+
async function validUseOfForAwaitOnAsyncIterable() {
108+
async function* yieldThingsAsynchronously() {
109+
yield 1;
110+
await new Promise(resolve => setTimeout(resolve, 1000));
111+
yield 2;
112+
}
113+
114+
for await (const promisedValue of yieldThingsAsynchronously()) {
115+
console.log(promisedValue);
116+
}
117+
}
118+
```
119+
120+
</TabItem>
121+
</Tabs>
122+
43123
## When Not To Use It
44124

45125
If you want to allow code to `await` non-Promise values.

‎packages/eslint-plugin/src/rules/await-thenable.ts‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { TSESLint } from '@typescript-eslint/utils';
1+
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';
22
import * as tsutils from 'ts-api-utils';
33

44
import {
@@ -10,8 +10,15 @@ import {
1010
nullThrows,
1111
NullThrowsReasons,
1212
} from '../util';
13+
import { getForStatementHeadLoc } from '../util/getForStatementHeadLoc';
1314

14-
export default createRule({
15+
type MessageId =
16+
| 'await'
17+
| 'forAwaitOfNonThenable'
18+
| 'removeAwait'
19+
| 'convertToOrdinaryFor';
20+
21+
export default createRule<[], MessageId>({
1522
name: 'await-thenable',
1623
meta: {
1724
docs: {
@@ -22,7 +29,10 @@ export default createRule({
2229
hasSuggestions: true,
2330
messages: {
2431
await: 'Unexpected `await` of a non-Promise (non-"Thenable") value.',
32+
forAwaitOfNonThenable:
33+
'Unexpected `for await...of` of a value that is not async iterable.',
2534
removeAwait: 'Remove unnecessary `await`.',
35+
convertToOrdinaryFor: 'Convert to an ordinary `for...of` loop.',
2636
},
2737
schema: [],
2838
type: 'problem',
@@ -62,6 +72,40 @@ export default createRule({
6272
});
6373
}
6474
},
75+
76+
'ForOfStatement[await=true]'(node: TSESTree.ForOfStatement): void {
77+
const type = services.getTypeAtLocation(node.right);
78+
if (isTypeAnyType(type)) {
79+
return;
80+
}
81+
82+
const asyncIteratorSymbol = tsutils.getWellKnownSymbolPropertyOfType(
83+
type,
84+
'asyncIterator',
85+
checker,
86+
);
87+
88+
if (asyncIteratorSymbol == null) {
89+
context.report({
90+
loc: getForStatementHeadLoc(context.sourceCode, node),
91+
messageId: 'forAwaitOfNonThenable',
92+
suggest: [
93+
// Note that this suggestion causes broken code for sync iterables
94+
// of promises, since the loop variable is not awaited.
95+
{
96+
messageId: 'convertToOrdinaryFor',
97+
fix(fixer): TSESLint.RuleFix {
98+
const awaitToken = nullThrows(
99+
context.sourceCode.getFirstToken(node, isAwaitKeyword),
100+
NullThrowsReasons.MissingToken('await', 'for await loop'),
101+
);
102+
return fixer.remove(awaitToken);
103+
},
104+
},
105+
],
106+
});
107+
}
108+
},
65109
};
66110
},
67111
});

‎packages/eslint-plugin/tests/docs-eslint-output-snapshots/await-thenable.shot‎

Lines changed: 60 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/eslint-plugin/tests/rules/await-thenable.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,28 @@ const doSomething = async (
198198
await callback?.();
199199
};
200200
`,
201+
{
202+
code: `
203+
async function* asyncYieldNumbers() {
204+
yield 1;
205+
yield 2;
206+
yield 3;
207+
}
208+
for await (const value of asyncYieldNumbers()) {
209+
console.log(value);
210+
}
211+
`,
212+
},
213+
{
214+
code: `
215+
declare const anee: any;
216+
async function forAwait() {
217+
for await (const value of anee) {
218+
console.log(value);
219+
}
220+
}
221+
`,
222+
},
201223
],
202224

203225
invalid: [
@@ -378,5 +400,73 @@ declare const obj: { a: { b: { c?: () => void } } } | undefined;
378400
},
379401
],
380402
},
403+
{
404+
code: `
405+
function* yieldNumbers() {
406+
yield 1;
407+
yield 2;
408+
yield 3;
409+
}
410+
for await (const value of yieldNumbers()) {
411+
console.log(value);
412+
}
413+
`,
414+
errors: [
415+
{
416+
messageId: 'forAwaitOfNonThenable',
417+
line: 7,
418+
endLine: 7,
419+
column: 1,
420+
endColumn: 42,
421+
suggestions: [
422+
{
423+
messageId: 'convertToOrdinaryFor',
424+
output: `
425+
function* yieldNumbers() {
426+
yield 1;
427+
yield 2;
428+
yield 3;
429+
}
430+
for (const value of yieldNumbers()) {
431+
console.log(value);
432+
}
433+
`,
434+
},
435+
],
436+
},
437+
],
438+
},
439+
{
440+
code: `
441+
function* yieldNumberPromises() {
442+
yield Promise.resolve(1);
443+
yield Promise.resolve(2);
444+
yield Promise.resolve(3);
445+
}
446+
for await (const value of yieldNumberPromises()) {
447+
console.log(value);
448+
}
449+
`,
450+
errors: [
451+
{
452+
messageId: 'forAwaitOfNonThenable',
453+
suggestions: [
454+
{
455+
messageId: 'convertToOrdinaryFor',
456+
output: `
457+
function* yieldNumberPromises() {
458+
yield Promise.resolve(1);
459+
yield Promise.resolve(2);
460+
yield Promise.resolve(3);
461+
}
462+
for (const value of yieldNumberPromises()) {
463+
console.log(value);
464+
}
465+
`,
466+
},
467+
],
468+
},
469+
],
470+
},
381471
],
382472
});

0 commit comments

Comments
 (0)