Hi @JavaScriptBach!

Could you expand a bit on what a a concrete use case looks like when this comes up, and why it would make sense in general to use for await there?

Btw for other readers, here is the sample code

const asyncIter: AsyncIterable<string> | Iterable<string> = [];
for await (const s of asyncIter) { }
export { }

Note also that there is a suggestion, but no autofix, since the fix would not be safe in general.

Ah sorry you're right, it is a suggestion, not an autofix.

Closing; it looks like everything here is working as intended.

My use case is that I have some library code that intentionally takes in a parameter typed AsyncIterable<T> | Iterable<T> and uses for await to handle both cases without repeating code. But I can disable the lint rule in that case as mentioned in the docs.

Well, the check for async iterables was just added this morning 🙂 #10008... so it may be "working as intended", but we're definitely listening closely at this time for feedback on whether the design is good in the first place or not!

I'd definitely invite you to continue the discussion here if you would like to make a case for an improvement to the behavior around for await checks.

This rule has a high batting average on my company's codebase, so overall I'm in favor of it.

It gives a false positive when operating on union types that may need for await, so perhaps you could consider make the rule not error in those cases? But I'm not familiar enough with type theory to know if that's an easy thing to detect or not.

This rule has a high batting average on my company's codebase, so overall I'm in favor of it.

❤️

It gives a false positive when operating on union types that may need for await, so perhaps you could consider make the rule not error in those cases? But I'm not familiar enough with type theory to know if that's an easy thing to detect or not.

This would definitely be simple to implement. I'm just looking to understand what the justification to do so is.

I think that this should be considered a bug personally because the rule does not report on a similar union case for non-iterables:

declare const maybePromise: Promise<number> | number;

await maybePromise;

playground

I think that this should be considered a bug personally because the rule does not report on a similar union case for non-iterables

That is true, however, await nonThenable is much more of a harmless noop than for await (const x of syncIterable) {}, which can have undesirable effects on error-handling.

in other words, it's quite intentional that the rule flags on for await used on sync iterables, even as a convenient way of unwrapping promises, since it has meaningfully different control flow from async iteration.

declare const arrayOfPromises: Array<Promise<number>>;
for await (const x of arrayOfPromises) { // bad
    x satisfies number;
}

But perhaps being somewhat lax with the union case is a worthwhile devX concession for useful real-world scenarios. That's the angle by which I'm the most likely to be persuaded.

(see also #8858 (comment))

Yeah I would be looking at it from the perspective that if you're attempting to iterate a AsyncIterable<string> | Iterable<string> then there is no option for you to separate out those types, right?

You could probably fashion some code that allows you to split things into two loops - one that's async and one that's not. But my question would be - why would you do that when that would require you to duplicate code.

Its the same reason we don't force people to do this for the regular promise union case:

const result = (value instanceof Promise) ? await value : value;

Sure this code is "more correct" but it's also not any better or any clearer.

So yeah I'd argue that from a DevX perspective flagging this is technically correct, but realistically it's fine to await this specific union case.

Yeah, I think I was radicalized by a couple of proposals (this and this) I read a few years ago to believe that it was specifically bad to mix async and sync iterators, but that's probably not terribly relevant here.

The main difference I see between the await maybePromise and for await (const {} of maybeAsyncIterable) cases is that the for await has meaningful differences to do with control flow.

So, whereas the difference between the following two lines

declare const value: Promise<string> | string;
const version1 = (value instanceof Promise) ? await value : value;
const version2 = await value;

is basically only observable via the incurring of an extra microtask tick, the for await vs for causes more standard control flow questions....
(TS Playground)

function resolveAfter(ms: number) {
  return new Promise<void>(resolve => { setTimeout(() => resolve(), ms) });
}

function * yield123Sync() {
    yield 1; yield 2; yield 3;
}

async function * yield123Async() { 
    await resolveAfter(100);
    yield 1; 
    await resolveAfter(100);
    yield 2; 
    await resolveAfter(100);
    yield 3;
}

function * yieldPromise123Sync() {
    yield resolveAfter(100).then(() => 1); 
    yield resolveAfter(100).then(() => 2); 
    yield resolveAfter(100).then(() => 3);
}

async function printValuesAsync<T>(iterable: Iterable<T> | AsyncIterable<T>) {
  const startTime = performance.now();
  console.log('starting to iterate (async flavor)');
  for await (const value of iterable) {
    console.log(value);
  }
  const endTime = performance.now();
  console.log(`done iterating (async flavor). Completed after ${endTime - startTime} ms`);

}


async function printValuesMixed<T>(iterable: Iterable<T> | AsyncIterable<T>) {
  const startTime = performance.now();
  console.log('starting to iterate (mixed flavor)');
  if (Symbol.asyncIterator in iterable){
    for await (const value of iterable) {
      console.log(value);
    }
  } else {
    for (const value of iterable) {
      console.log(value);
    }
  }
  const endTime = performance.now();
  console.log(`done iterating (mixed flavor). Completed after ${endTime - startTime} ms`);
}

async function main() {
  await printValuesAsync(yield123Sync()); // ~ 1 ms
  await printValuesAsync(yield123Async()); // ~309 ms
  await printValuesAsync(yieldPromise123Sync()); // ~311 ms

  console.log('compare to');

  await printValuesMixed(yield123Sync()); // ~2.7 ms
  await printValuesMixed(yield123Async()); // ~311 ms
  // the control flow here is quite different.
  await printValuesMixed(yieldPromise123Sync()); // ~5 ms
}

main()

So, anyway, what I'm really interested to understand, and don't currently understand, is - why would one end up with AsyncIterable<T> | Iterable<T> in the first place, and what control flow would a user of such an API expect to happen if passing a possibly async-, possibly sync- iteratable to a Promise-returning function that takes either? Should it iterate their promises synchronously, then resolve, or await each promise sequentially?

If it should be iterated synchronously, then I would say the API is better expressed as a non-promise-returning API in the first place, in which case the implementation will not be using for await.

If it should be iterated asynchronously/sequentially, then I would say that for await has incorrect error handling that permits floating promises, and one should consider instead some abomination like await Promise.all(promises.map(async () => { /* do something after each promise */ }); or similar.

This is all to say, I'm certainly open to there being lenient handling of unions containing AsyncIterable<T>, I just really would like to have a good, real-world use to be able to point to as an example of why it's permitted if we do permit it (partially for my own interest! 🙂 ). I'm only marginally opposed by default in lieu of such an example.

My code uses Iterable<T> | AsyncIterable<T> for the same reason as T | Promise<T>: some implementations might be sync, the union type avoids unnecessary async keywords (and lint errors) on them.

From reading the above comments, what I understand is that the control flows of Iterable<Promise<T>> and AsyncIterable<T> are different. But in my usage T is a concrete type (and I guess in generic usages, T would have a type constraint), so our functions will only accept Iterable<T> and AsyncIterable<T>, but not Iterable<Promise<T>>, thus the problem won't happen.

So, the only difference between for await ... of value blindly and Symbol.asyncIterator in value ? for await ... of value : for ... of value is the extra microtasks, which I also don't care.

Hello I just opened a PR to fix this issue,

However I don't know if that will resolve it for your example

const asyncIter: AsyncIterable<string> | Iterable<string> = [];
for await (const s of asyncIter) { }

I think Typescript will automatically deduct that asyncIter is only an array, and the variable won't be treated as a Union type...

Hello I just opened a PR to fix this issue,

However I don't know if that will resolve it for your example

const asyncIter: AsyncIterable<string> | Iterable<string> = [];
for await (const s of asyncIter) { }

I think Typescript will automatically deduct that asyncIter is only an array, and the variable won't be treated as a Union type...

Yeah, that's just TS being clever and narowing asyncIter based off its actual value, rather than treating it as the type in the type annotation. Technically we could write the repro more correctly as

declare const asyncIter: AsyncIterable<string> | Iterable<string>;

for await (const s of asyncIter) { }

export { }

or

async function foo(asyncIter: AsyncIterable<string> | Iterable<string>) {
  for await (const s of asyncIter) { }
}