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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

support async generators with ->>* #1063

Merged
merged 1 commit into from Aug 14, 2018

Conversation

Projects
None yet
4 participants
@rhendric
Collaborator

rhendric commented Jul 30, 2018

@pepkin88 brought to my attention that async generators are coming down the JS pipe (and already supported by default in Node.js 10.7), and I think they're pretty easy to add at this point and I see no reason to wait. I said I wouldn't prioritize more features for 1.6 but this is a pretty minor and obvious combination of two existing features, so I'm giving myself an exception. 馃槃 Open for comments for two weeks, to be merged on August 13 if there are no opposing voices.

@gkovacs, @summivox, as sponsors of the original async work, if you think I've missed anything here I'm interested in hearing about it.

@vendethiel

This comment has been minimized.

Contributor

vendethiel commented Jul 30, 2018

Beautiful ascii art, as expected

@pepkin88

This comment has been minimized.

Contributor

pepkin88 commented Jul 30, 2018

Could be more tests from generators.ls.

For example:

# yield from in let
first = ->>*
  i = 0
  loop => yield await Promise.resolve i++
second = ->>*
  let
    await Promise.resolve 1
    yield from first!
list = second!
for let i to 3
  {value} <- list.next!then
  eq value, i

(a modified test from generators.ls with an addition of a let block)
This passes, but only due to recent changes.

However this one doesn't:

# in switch
f7 = (x) ->*
    y = switch x
    | true => yield await Promise.resolve 1
    | _    => yield await Promise.resolve 2
    y
g7 = f7 true
g7.next!then -> eq 1 it.value

(a modified test from generators.ls)

Speaking of which, what do you think about my idea from #1060 (comment) to rewrite Node::compile-closure to use Call.let? That way we would avoid duplicating a similar behavior. Moreover, Call.let produces more optimized code, because the wrapper doesn't have to be a (async) generator function, if the body doesn't have await or yield.


Edit: Pardon my foolishness, I forgot to change the arrow ^_^' The test passes.

support async generators with ->>*
Also support the more verbose `async function* named-fn` option.
@rhendric

This comment has been minimized.

Collaborator

rhendric commented Jul 30, 2018

There is a test with a let block already in there (which indeed does fail if I revert your fix). I added a test to exercise yield from, but I don't think it's necessarily valuable to repeat all of the generator.ls tests in async form.

Re your compile-closure proposal, if you want to go forward with that, I'd review it. Maybe the two functions should be unified, but at present both do optimizations that the other does not (Call.let always produces a .call invocation, whereas Node::compile-closure only does so if this is used). So I don't think it's a clean case of one should use the other, but if you want to try merging them, be my guest.

@summivox

This comment has been minimized.

Contributor

summivox commented Jul 30, 2018

LGTM

@pepkin88

This comment has been minimized.

Contributor

pepkin88 commented Jul 30, 2018

@rhendric Should I do it on your branch or on master?

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Jul 30, 2018

Master, unless you expect it to depend on this work for some reason.

@pepkin88

This comment has been minimized.

Contributor

pepkin88 commented Jul 30, 2018

After some thought I think I'll postpone that indefinitely - the gain isn't that big and Call.let isn't as compatible as I thought, it would require some custom handling of arguments. So I'll leave it for now.

And regarding async generators, looks great 馃憤

@rhendric rhendric merged commit 367c6da into gkz:master Aug 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment