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

Update Q.async to work with current ES6 generators #288

Merged
merged 1 commit into from May 8, 2013

Conversation

wingo
Copy link
Collaborator

@wingo wingo commented May 8, 2013

ES6 generators are slightly different from what has been shipped in
SpiderMonkey. For our purposes, the differences are:

  • they use function* as a token;
  • they box their return value in a { value: , done: }
    object; and
  • they can return a value.

They are close enough to SpiderMonkey that Q.async can support both.
This commit checks for legacy generators at Q startup, and expects
unboxed values in that case.

This commit also updates the examples. In particular, the wishful
thinking and shim examples go away, as they are handled naturally with
the new specification. A copy of the legacy examples has been moved to
the examples/async-generators/js-1.7/ folder.

ES6 generators are slightly different from what has been shipped in
SpiderMonkey.  For our purposes, the differences are:

  * they use function* as a token;
  * they box their return value in a { value: <any>, done: <bool> }
    object; and
  * they can return a value.

They are close enough to SpiderMonkey that Q.async can support both.
This commit checks for legacy generators at Q startup, and expects
unboxed values in that case.

This commit also updates the examples.  In particular, the wishful
thinking and shim examples go away, as they are handled naturally with
the new specification.  A copy of the legacy examples has been moved to
the examples/async-generators/js-1.7/ folder.
@wingo
Copy link
Collaborator Author

wingo commented May 8, 2013

I tested this with FireFox and the js-1.7 directory. For ES6 I tested with a fresh Chrome build, hacked to use bleeding_edge v8, run with --harmony.

@domenic
Copy link
Collaborator

domenic commented May 8, 2013

This is one of the most exciting things I've seen :D. Amazing work sir! Will let @kriskowal do the honors of merging, but I can't wait to start using these.

@kriskowal
Copy link
Owner

I actually am wondering whether I might beg a code review from @erights. Is this consistent with the direction of the language? Is this mature?

@erights
Copy link
Collaborator

erights commented May 8, 2013

I'll try. But I make no, uh, promises about when I'll get to it. If I
haven't after a reasonable time, please don't block on me.

On Wed, May 8, 2013 at 1:15 PM, Kris Kowal notifications@github.com wrote:

I actually am wondering whether I might beg a code review from @erightshttps://github.com/erights.
Is this consistent with the direction of the language? Is this mature?


Reply to this email directly or view it on GitHubhttps://github.com//pull/288#issuecomment-17630900
.

Text by me above is hereby placed in the public domain

Cheers,
--MarkM

@kriskowal
Copy link
Owner

Thanks for this article @andywingo, http://wingolog.org/archives/2013/05/08/generators-in-v8

@kriskowal
Copy link
Owner

I’m inclined to land this before a thorough review and deal with the consequences as they come. I expect that there will be wonky issues with CSP and the eval to feature test. Not necessarily that it will fail generally, but that it will fail when it should pass. I respect the decision to completely separate and replicate the inner algorithm for Q.async so it’s easier to delete the old path. It will be worth noting that we will not remove the shim when Firefox upgrades its generators but when no one is using the old generators on Firefox, presumably a much later date, unless we force forward and leave such users stranded in a backward version of Q (a good option too).

@erights
Copy link
Collaborator

erights commented May 8, 2013

+1

@wingo
Copy link
Collaborator Author

wingo commented May 8, 2013

It almost goes without stating, but not quite, so just to mention I'm happy to make any changes you suggest. I'm not really familiar with Q and am actually quite a bad JS programmer ;) so it's likely I didn't do things quite right.

Will take a look in the euro-morning. Cheers.

@kriskowal
Copy link
Owner

I don’t see anything that we can change, unless there’s a way to feature-test for Harmony generators without using eval. I will land this as-is.

@kriskowal kriskowal merged commit d69254e into kriskowal:master May 8, 2013
@kriskowal
Copy link
Owner

@domenic This is prepped for release. Let me know if you want anything else in v0.9.4.

@domenic
Copy link
Collaborator

domenic commented May 8, 2013

@kriskowal if you could look at #238 and maybe #278, a lot of people are getting hurt by those.

@wingo wingo deleted the harmony-generators branch May 8, 2013 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants