Support async generators

[dougmoscrop]

Not sure if there's interest in landing this - I guess it's a question of whether you want to drop IE11 support or introduce babel or something, so there's two commits - one just uses async/await, but the grunt task fails. The other (most recent) uses Promise.resolve and the grunt task works.

[dougmoscrop]

I'll add tests once we confirm that this is wanted

[vqvu]

I'm happy to accept this. In addition to adding tests, can you also 1. Revert the generated files (dist/* and docs/index.html). They're regenerated upon release, so no need to have them in the PR. 2. Move the implementation to a new top-level function called fromAsyncGenerator. The current implementation auto resolves promises, which breaks backwards compatibility with the current behavior for generators. You can leave the changes you had to make to the tests so far. They're more correct anyway. 3. Stop using Promise.resolve so we don't depend on the Promise global. Just use it.next().then(...).

…

On Wed, Jun 5, 2019, 10:35 PM Doug Moscrop ***@***.***> wrote: I'll add tests once we confirm that this is wanted — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#682?email_source=notifications&email_token=ABRDEZMRF5PL5N6L6K262TTPY7MKLA5CNFSM4HT4BPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXADLCI#issuecomment-499135881>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABRDEZI32YFUAWOXCEBGOE3PY7MKLANCNFSM4HT4BPXA> .

[dougmoscrop]

Yes, no problem, thanks.

[dougmoscrop]

Actually just to be clear, is the desired interface such that you do something like _.fromAsyncGenerator(...)?

It's not clear to me how I can detect within _(thing) that thing is an async generator that I should resolve the promises on, I think maybe I can detect if the result from next() is thenable, I'll test around but it depends on whether or not you want this to be supported from _() directly or only with a helper

[vqvu]

Correct. The interface should just be `_.fromAsyncGenerator(...)`.

…

On Fri, Jun 14, 2019, 2:56 AM Doug Moscrop ***@***.***> wrote: Actually just to be clear, is the desired interface such that you do something like _.fromAsyncGenerator(...)? It's not clear to me how I can detect within _(thing) that thing is an async generator that I should resolve the promises on, I think maybe I can detect if the result from next() is thenable, I'll test arond but it depends on whether or not you want this to be supported from _() directly or only with a helper — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#682?email_source=notifications&email_token=ABRDEZKOJJGEVMD2V35V7TDP2KC4PA5CNFSM4HT4BPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXURC3Y#issuecomment-501813615>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABRDEZK4GU4BHNQXWGC26ALP2KC4PANCNFSM4HT4BPXA> .

[dougmoscrop]

Sorry it took so long.

I don't intend to be argumentative, but you mentioned that it's currently resolving promises - I don't believe it is, in fact I think I wrote test that shows this. The only promise that gets resolved is the one that comes from .next() (if it returns one -- generators don't, async generators do), not iterElem.value -- it is left unresolved (if userland send an unresolved promise to the stream)

I only did this since I personally prefer to keep the interface as simple as possible (_(most_things)) and consistent (no _.fromGenerator exists)

The current impl is a bit ugly because I got rid of the "hoisting" of everything in to a Promise(), and instead I have to detect if the result of it.next() is 'thenable' (promise would do this for us)

Let me know what you think?

[vqvu]

Can you also update update the documentation at the top of index.js to add an entry for Asynchronous Iterator? Link to https://github.com/tc39/proposal-async-iteration.

[vqvu]

Is this still necessary? I tried pulling your changes and ran the test without setImmediate, and they all passed.

[dougmoscrop]

Yes, sorry that comment is poorly phrased: I have "real" code that misbehaves when this is not present, but the tests are fine; I hope to add a test that demonstrates the problem, I just have to come up with one.

[dougmoscrop]

A red herring I think, it was because of tomfoolery in some other part of code (I was using observe().forEach() without handling errors and that cause some.. oddness).

I've removed this.

[vqvu]

You're absolutely right. I was confused about what the result of calls to next() would be. I incorrectly assumed that async generators return objects of type {value: Promise<T>, done: boolean}, but they actually return objects of type Promise<{value: T, done: boolean}>.

I have no more major concerns.

[dougmoscrop]

OK, I took a stab at the docs and removed the _.setImmediate weirdness. Let me know if there's anything else!

[vqvu]

LGTM. Thanks for the contribution!