Closed lsmith closed 11 years ago
I like it!
Tests look great! Do you want some help writing them?
I can't find the specific source for this claim (I'll keep looking later), but I was under the impression that all promise handlers were executed asynchronously (i.e. wrapped in a setTimeout(fn,0)
).
Even if I can't find the specific source, I think it would be a good thing to implement. It improves consistency for asynchronous/synchronous function calls and should have a similar effect to the AsyncQueue module for processing-heavy promise methods.
Aside from that and a few code nitpicks, I think this looks awesome!
@TeslaNick The CommonJs Promises B proposal requires callbacks to be executed asynchronously. http://wiki.commonjs.org/wiki/Promises/B
This seems to be a fancy implementation of the Promises A proposal which does not have that requirement. http://wiki.commonjs.org/wiki/Promises/A
The Q API is a fancy implementation of Promises B. https://github.com/kriskowal/q The Q API uses tricks similar to gallery-soon to achieve asynchronicity. http://yuilibrary.com/gallery/show/soon
I agree that it would be nice if callbacks were executed asynchronously but setTimeout is a pretty huge performance waster.
@solmsted Is it? I couldn't find anything that suggested that setTimeout introduced performance overhead and since it's used in semi-real-time code (e.g. animation), I don't think that whatever performance hit it incurs would be a problem for the cases promises are likely to be used in. I think using alternatives (setImmediate and process.nextTick come to mind) when they're available is a reasonable compromise.
@TeslaNick Try this in your favorite browsers: http://jsperf.com/setimmediate-test/5 In many cases, the performance differences are significant.
This is why I made gallery-soon.
In terms of promises, if you have a couple things chained together, unless you're on a mobile device or laptop where the minimum setTimeout timer resolution is increased, you're not likely to notice a difference. As you scale to a Node server performing tons of IO with real-time data and maybe dozens of promises nested within dozens more, the performance issues become a problem.
@solmsted Looks good, though I don't want to bake that much code into this module for ms shaving that will largely be beneficial in long chains of synchronous ops or in Node.js. I'll add a sniff for Y.soon.
It seems like consensus is that all then()
callbacks should be async. It looks like jQuery's then()
doesn't enforce async. I'm satisfied saying that this implementation is based on Promises/A with deviations.
In reading over the API discussion linked from the Promises/A wiki entry, there was nothing specific about rationale for progress being included in then()
's signature. While I do view jq's implementation as a strong indicator of convention, I'm feeling inclined to dig in my heels on the progress point, and let feedback from implementation-land convince me to recant my heathen ways.
@juandopazo Regarding tests, um, yeah! MOAR TESTS. I'll slam out as many as I can in the next couple hours, then if you want to work from the bottom, we won't collide if working in parallel.
I like Y.soon
, specially if it uses nextTick in Node. I think we'll be seeing more and more APIs go full async in the future. Even Object.observe
is being drafted as async, so we'd better get used to it.
Deferred is targeted to serve in the role of transaction layer (second layer in https://gist.github.com/2375130 per https://github.com/yui/yui3/wiki/Ongoing-Development-Discussions).
Feedback welcome while I write tests.