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

Using for-await-of should resolve the Promise? #21115

Closed
luthfianto opened this issue Jan 10, 2018 · 12 comments · Fixed by #24474
Closed

Using for-await-of should resolve the Promise? #21115

luthfianto opened this issue Jan 10, 2018 · 12 comments · Fixed by #24474
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@luthfianto
Copy link

TypeScript Version:

Code

declare function require(name:string);

(<any> Symbol).asyncIterator = Symbol.asyncIterator || Symbol.for("Symbol.asyncIterator");

const fetchlike = function(url) {
  // return new pending promise
  return new Promise((resolve, reject) => {
    // select http or https module, depending on reqested url
    const lib = url.startsWith('https') ? require('https') : require('http');
    const request = lib.get(url, (response) => {
      // handle http errors
      if (response.statusCode < 200 || response.statusCode > 299) {
         reject(new Error('Failed to load page, status code: ' + response.statusCode));
       }
      // temporary data holder
      const body = [];
      // on every content chunk, push it to the data array
      response.on('data', (chunk) => body.push(chunk));
      // we are done, resolve promise with those joined chunks
      response.on('end', () => resolve(body.join('')));
    });
    // handle connection errors of the request
    request.on('error', (err) => reject(err))
    })
};

async function example() {
  const arrayOfFetchPromises = [
    fetchlike('http://example.com'),
    fetchlike('http://example.com'),
  ];

  // Regular iterator:
  for (const item of arrayOfFetchPromises) {
    console.log(item); // Logs a promise
  }

  // for-await-of iterator:  
  for await (const item of arrayOfFetchPromises) {
    console.log(item); // Logs a response
  }
}

example()

tsc --lib es2017,esnext.asynciterable,dom Untitled-1.ts && node Untitled-1.js

Actual behavior:

item is a Promise in the for-await-of iterator block

imgur

Doing this causes error:

  for await (const item of arrayOfFetchPromises) {
    console.log(item.length); // TS error
  }
  for await (const item of arrayOfFetchPromises) {
    console.log((item as any).length);
    // not error because typecasted to any, logs valid result
  }

Expected behavior:

In the for-await-of iterator block,item should not be a Promise, because it's the actual value instead of a Promise

@ghost
Copy link

ghost commented Jan 10, 2018

for await doesn't take an array of promises, it takes an AsyncIterable. See https://github.com/tc39/proposal-async-iteration
For what you want I think you can just do:

for (const item of arrayOfPromises) {
    console.log(await item);
}

or: for (const item of await Promise.all(arrayOfPromises)) { console.log(item); }

@ghost ghost added the Question An issue which isn't directly actionable in code label Jan 10, 2018
@Jessidhia
Copy link

Jessidhia commented Jan 11, 2018

@andy-ms An array of promises should work.

The for/of head evaluation calls GetIterator, which will either return the result of evaluating [Symbol.asyncIterator]() if [Symbol.asyncIterator] exists, or the result of [Symbol.iterator]() passed to CreateAsyncFromSyncIterator. This means that anything with a [Symbol.iterator]() is an AsyncIterable, just maybe not an optimally concurrent one.

The %AsyncFromSyncIteratorPrototype%.next will always wrap the sync iterator result in a promise (step 12), which ultimately means that, if the argument to for/of is an iterable, each of the values it yields will be wrapped in a Promise, which is a valid way of consuming an iterable of Promises (or PromiseLike, or even plain values).


What you can't give as an argument to for await of is a Promise to an iterable (or a Promise to anything). You have to await that Promise first with for await (LeftHandSideExpression of await promiseToIterable) {}.

@ghost
Copy link

ghost commented Jan 11, 2018

OK, based on Perform ! Call(valueWrapperCapability.[[Resolve]], undefined, « nextValue »)., it does sound like the .value of the iterator result is awaited in addition to the result itself. So maybe our emit should handle this situation.

@ghost ghost added Bug A bug in TypeScript and removed Question An issue which isn't directly actionable in code labels Jan 11, 2018
@luthfianto
Copy link
Author

Thanks, so how to fix this? Does simply changing /src/lib/esnext.asynciterable.d.ts#L14 Promise<IteratorResult<T>> -> IteratorResult<T> fix this?

@ghost
Copy link

ghost commented Jan 11, 2018

I think we need to ensure the emit would work with that as well. CC @rbuckton

@mhegazy mhegazy added this to the TypeScript 2.8 milestone Jan 11, 2018
@Jamesernator
Copy link

Jamesernator commented Jan 13, 2018

Just as a note Chrome's semantics are correct in regards to the current spec so you can try things out in there.

Here's some sample outputs that can happen:

Iterating an async generator:

async function* gen() {
    yield 1
    yield new Promise(resolve => setTimeout(resolve, 1000, 2)
    yield 3
}

for await (const item of gen()) {
    console.log(item)
}
// -> 1
// -> 2 // After 1 second
// -> 3 // Also after 1 second

Iterating an iterable:

function* gen() {
    yield 1
    yield new Promise(resolve => setTimeout(resolve, 1000, 2)
    yield 3
}

for await (const item of gen()) {
    console.log(item)
}
// Exactly the same as above
// -> 1
// -> 2
// -> 3

Iterating an async iterable that actually emits Promises in the value field (NOTE That is not a valid async iterable but this isn't enforced and is is still "usable" in some sense).

const values = [
    _ => 1,
    _ => new Promise(resolve => setTimeout(resolve, 1000, 2)),
    _ => Promise.resolve(3)
]
const iterator = {
    i: 0,
    async next() {
        if (this.i >= 3) {
            return { done: true }
        }
        const value = values[this.i]()
        this.i += 1
        return { value, done: false }
    },

    [Symbol.asyncIterator]() {
        return this
    }
}

for await (const item of iterator) {
    console.log(item)
}

// -> 1
// -> Promise{<pending>}
// -> Promise{<resolved>: 3}

Iterating an async iterable that doesn't give actual Promises from next() will still be wrapped in Promises:

// Same as previous but .next is not asynchronous
const values = [
    _ => 1,
    _ => new Promise(resolve => setTimeout(resolve, 1000, 2)),
    _ => Promise.resolve(3)
]
const iterator = {
    i: 0,
    next() {
        if (this.i >= 3) {
            return { done: true }
        }
        const value = values[this.i]()
        this.i += 1
        return { value, done: false }
    },

    [Symbol.asyncIterator]() {
        return this
    }
}

for await (const item of iterator) {
    console.log(item)
}

// Same as above
// -> 1
// -> Promise{<pending>}
// -> Promise{<resolved>: 3}

Note that is even more confusing in the presence of yield* because it implicitly awaits the value field of the iterator result so if we had either of the latter cases with yield* we get what we had in the first case:

const values = [
    _ => 1,
    _ => new Promise(resolve => setTimeout(resolve, 1000, 2)),
    _ => Promise.resolve(3)
]
const iterator = {
    i: 0,
    next() {
        if (this.i >= 3) {
            return { done: true }
        }
        const value = values[this.i]()
        this.i += 1
        return { value, done: false }
    },

    [Symbol.asyncIterator]() {
        return this
    }
}

async function* it() {
    yield* iterator
}

for await (const item of it()) {
    console.log(item)
}

// -> 1
// -> 2 // After 1 second
// -> 3 // Also after 1 second

With these outputs in mind there's two things to note, one is that a for-await-of loop can take either an AsyncIterable or a SyncIterable but they're not interchangeable. As such we can type the for-await-of like such:

  • When for await (varName of <object>) is given an AsyncIterable<T> then varName will always be of type T even if T is Promise<A>.

  • When for await (varName of <object>) is given a SyncIterable<Promise<T>> then varName will be of type T.

  • When for await (varName of <object>) is given a SyncIterable<T> then varName will be of type T.

Also whether or not allowing AsyncIterable<Promise<T>> or allowing AsyncIterator<T>.next to return a { value: T, done: boolean } is a good idea I'm not sure, but given that the spec says that AsyncIterable<Promise<T>> is not a valid AsyncIterable it's probably better to disallow it despite being passed through in for-await-of. But doing this would probably need the ability to haves negated types like in #7993.

@simonbuchan
Copy link

So for now I have:

// Things you can "for await of" on
type ForAwaitOfable<T> =
  | ReadonlyArray<T>
  // | ReadonlyArray<Promise<T>> // https://github.com/Microsoft/TypeScript/issues/21115
  | Iterable<T>
  // | Iterable<Promise<T>>
  | AsyncIterable<T>
  ;

Am I missing anything?

If emit is an issue then I would still like the typing change, and pass-through when target is esnext (es2018?), otherwise emit an error like "for await of an iterable of '${promise-like type T}' requires target: esnext" similar to non --downlevelIteration

@rbuckton
Copy link
Member

rbuckton commented Feb 1, 2018

We will need to verify the emit behavior against the final version of the async iteration spec and the related test-262 tests.

@rbuckton
Copy link
Member

rbuckton commented May 9, 2018

I've looked into this more and it seems the only issue as of TypeScript 2.9 is the type we report. The value at runtime is correct..

@luthfianto
Copy link
Author

luthfianto commented May 10, 2018

So, does this

const ar = [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)];

async function main() {
    for await (const a of ar) {
        console.log(a);
    }
}

correctly translates to

var __asyncValues = (this && this.__asyncValues) || function (o) {
    if (!Symbol.asyncIterator) throw new TypeError("Symbol.asyncIterator is not defined.");
    var m = o[Symbol.asyncIterator];
    return m ? m.call(o) : typeof __values === "function" ? __values(o) : o[Symbol.iterator]();
};

const ar = [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)];
async function main() {
    try {
        for (var ar_1 = __asyncValues(ar), ar_1_1; ar_1_1 = await ar_1.next(), !ar_1_1.done;) {
            const a = await ar_1_1.value;
            console.log(a);
        }
    }
    catch (e_1_1) { e_1 = { error: e_1_1 }; }
    finally {
        try {
            if (ar_1_1 && !ar_1_1.done && (_a = ar_1.return)) await _a.call(ar_1);
        }
        finally { if (e_1) throw e_1.error; }
    }
    var e_1, _a;
}

in Javascript?

And does simply changing /src/lib/esnext.asynciterable.d.ts#L14 Promise<IteratorResult<T>> -> IteratorResult<T> fix this?

@Jamesernator
Copy link

Jamesernator commented May 10, 2018

@rbuckton @rilut No it's still not correct, I tested in the latest version in the git repository and this example is still wrong:

const values = [
    _ => 1,
    _ => new Promise(resolve => setTimeout(resolve, 1000, 2)),
    _ => Promise.resolve(3),
];

const iterator = {
    i: 0,
    async next() {
        if (this.i >= 3) {
            return { done: true };
        }
        const value = values[this.i]();
        this.i += 1;
        return { value, done: false };
    },

    [Symbol.asyncIterator]() {
        return this;
    }
}

async function main() {
    for await (const item of iterator) {
        console.log(item);
    }
}

main();

// -> 1
// -> Promise{<pending>}
// -> Promise{<resolved>: 3}

Currently TypeScript still just prints out the numbers:

1
2 // After 1 second
3 // Also after 1 second

Which is not correct according to the spec, the correct behaviour should be exactly as in the comment above. You can even try this out now in Node 10 or Chrome both with no flags both of which have the correct output based on the specification.

@rilut A more correct version would be this where __asyncValues actually returns an AsyncFromSyncIterator object as defined in the spec:

function __asyncValues() {
    if (!Symbol.asyncIterator) throw new TypeError("Symbol.asyncIterator is not defined.");
    var m = o[Symbol.asyncIterator];
    // Where AsyncFromSyncIterator implements the .value await-ing logic
    // like in the spec
    return m ? m.call(o) : typeof __values === "function" ? __values(o) : new AsyncFromSyncIterator(o[Symbol.iterator]());
}

async function main() {
    try {
        for (var ar_1 = __asyncValues(ar), ar_1_1; ar_1_1 = await ar_1.next(), !ar_1_1.done;) {
            // No await-ing the value
            const a = ar_1_1.value;
            console.log(a);
        }
    }
    catch (e_1_1) { e_1 = { error: e_1_1 }; }
    finally {
        try {
            if (ar_1_1 && !ar_1_1.done && (_a = ar_1.return)) await _a.call(ar_1);
        }
        finally { if (e_1) throw e_1.error; }
    }
    var e_1, _a;
}

@jesseschalken
Copy link
Contributor

Note to anyone who has found this issue wondering why it isn't fixed in the current stable (3.1.6): The fix is actually #27271, not #24474, and is awaiting release in 3.2.0-rc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants