Add the `spawn` function for ES6 generators #306

Merged
merged 1 commit into from Jun 4, 2013

Conversation

Projects
None yet
3 participants
@jlongster
Collaborator

jlongster commented May 31, 2013

It's very, very common to use ES6 generators at the top-level when you don't want to continue chaining promises, and you want any errors to be thrown. The key factor is that since all errors are suppressed, even language-level ones, it's too easy to forget to call done() when using generators since promises are more implicit now.

I'd say the most common case for ES6 generators is at the top-level that consume other libraries.

I will add more tests and be more thorough about this if this is at least approved to be merged. (I added a test within examples though)

@@ -0,0 +1,27 @@
+var Q = require('../../q');
+
+function delay(millis, answer) {

This comment has been minimized.

@domenic

domenic Jun 2, 2013

Collaborator

You can just use Q.delay, no?

@domenic

domenic Jun 2, 2013

Collaborator

You can just use Q.delay, no?

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 2, 2013

Collaborator

+1 to the idea, both on the level of simple sugar for Q.async(...)() and also on the level of tracking down unhandled rejections better. I've definitely accidentally created unhandled rejections myself via just simple generator usage, so it could use some work. Approved, if you can add a few tests showing that it does what it says it does (which should be nice and easy with a combination of Q.onerror and the new Q.getUnhandledRejections() etc.).

Collaborator

domenic commented Jun 2, 2013

+1 to the idea, both on the level of simple sugar for Q.async(...)() and also on the level of tracking down unhandled rejections better. I've definitely accidentally created unhandled rejections myself via just simple generator usage, so it could use some work. Approved, if you can add a few tests showing that it does what it says it does (which should be nice and easy with a combination of Q.onerror and the new Q.getUnhandledRejections() etc.).

@kriskowal

This comment has been minimized.

Show comment
Hide comment
@kriskowal

kriskowal Jun 2, 2013

Owner

Are you sure we should have an implicit done() on the end of spawn()? Do we imagine that this would only be used at the top level of some promise work, and we would only use async to do work that might return a promise?

Owner

kriskowal commented Jun 2, 2013

Are you sure we should have an implicit done() on the end of spawn()? Do we imagine that this would only be used at the top level of some promise work, and we would only use async to do work that might return a promise?

+ */
+Q.spawn = spawn;
+function spawn(makeGenerator) {
+ Q.done(Q.async(makeGenerator)());

This comment has been minimized.

@kriskowal

kriskowal Jun 2, 2013

Owner

Internally, you can just say done and async instead of Q.done and Q.async.

@kriskowal

kriskowal Jun 2, 2013

Owner

Internally, you can just say done and async instead of Q.done and Q.async.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 2, 2013

Collaborator

@kriskowal IMO that's the correct division of responsibilities, although I hadn't seen it put so clearly until you did so :). Ideally, with generators, we should be able to completely avoid unhandled rejections. The only time they should be possible is if you use a promise-returning side-effecting function but forget that it's promise-returning, i.e. you just fire-and-forget it. And async side-effecting functions are the perfect use case for spawn, so if spawn calls done for you... seems good.

Collaborator

domenic commented Jun 2, 2013

@kriskowal IMO that's the correct division of responsibilities, although I hadn't seen it put so clearly until you did so :). Ideally, with generators, we should be able to completely avoid unhandled rejections. The only time they should be possible is if you use a promise-returning side-effecting function but forget that it's promise-returning, i.e. you just fire-and-forget it. And async side-effecting functions are the perfect use case for spawn, so if spawn calls done for you... seems good.

kriskowal added a commit that referenced this pull request Jun 4, 2013

@kriskowal kriskowal merged commit 6a34b92 into kriskowal:master Jun 4, 2013

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Jun 4, 2013

Collaborator

Oh man, I didn't realize I wasn't watching this repo so I didn't see any of your comments. I think github should show notifications for PRs opened by you...

This has been merged, so I assume that you cleaned up the code afterwards? Do you want me to write any more tests?

Collaborator

jlongster commented Jun 4, 2013

Oh man, I didn't realize I wasn't watching this repo so I didn't see any of your comments. I think github should show notifications for PRs opened by you...

This has been merged, so I assume that you cleaned up the code afterwards? Do you want me to write any more tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment