jmar777 / suspend

Callback-free control flow for Node using ES6 generators.
548 stars 22 forks source link

make sync callbacks explicit #10

Closed helmus closed 10 years ago

helmus commented 10 years ago

I'm submitting this PR in response to 1bda954d0da1c508918cdaab59234bef299262b6 Suspend is now using 2 cycles to handle off async callbacks in favour of normalizing sync callbacks. I'm sure the performance impact of this is minimal but this doesn't feel like the right thing to do.

Adding a sync resumator option to the resume callback fixes this by allowing to signal that the callback is synchronous. This also provides better code documentation, when reading through a suspend function a developer immediately notices if a callback is synchronous or asynchronous.

You could argue that sometimes you don't know if a callback is going to be async or sync, this is really more an inconsistency in the library you are using. But this can of course be fixed by just using the sync resumator which will handle both cases fine.

This PR will break synchronous promises, you can fix this like this:

yield doStuffEventually.then(r.sync);

I don't really know a better way to fix it at this moment. Any feedback is appreciated.

jmar777 commented 10 years ago

I agree that it's not ideal to always setImmediate() the resumer, but I do want to keep the sync/async callback resolution transparent if at all possible. Part of the goal is to let you not worry about it, so introducing resume.sync would violate that. I'll have to play around a bit, but there may be away to detect that with a state tracker somewhere in between https://github.com/jmar777/suspend/blob/master/lib/suspend.js#L65 and https://github.com/jmar777/suspend/blob/master/lib/suspend.js#L13. It would still require a call to setImmediate() to flip the state tracker back, but at least it would happen prior to resume actually being invoked (in the majority / async case, anyway).

jmar777 commented 10 years ago

Closing - please see this comment for details. Thanks again!