New module to add Y.Deferred, Y.Promise, and Y.when() #36
Conversation
With some real tests, some stubbed tests. Basic functionality seems to be working, but I haven't been very rigorous about it yet.
New version uses prototypes and gentleman's privates, and is broken into two pieces: 1. `deferred` which offers Y.Deferred and Y.Promise with only deferred.then(), resolve(), reject(), promise(), and getStatus(), and promise.then() and promise(). 2. `deferred-extras` which adds deferred/promise.onProgress() and deferred.notify() for progress updates, an promise.wait() to insert a delay in the chained sequence. Also added `deferred-when` which creates a `Y.when()` method to wrap multiple operations and returns a promise that will be resolved when all individual operations complete.
Split up the code into two modules: deferred - core implementation deferred-extras - +onProgress/notify, wait, and Y.when
| @module deferred | ||
| @since 3.7.0 | ||
| **/ | ||
| var slice = [].slice, |
There was a problem hiding this comment.
Array.prototype.slice will avoid creating a throwaway array. Nitpicking, I know.
|
I like it! |
| @return {Promise} The promise of a new Deferred wrapping the resolution | ||
| of either "resolve" or "reject" callback | ||
| **/ | ||
| then: function (callback, errback) { |
There was a problem hiding this comment.
Don't you want it to accept an array of callbacks?
There was a problem hiding this comment.
I didn't think it necessary. But I'm open to a community vote. An array of callbacks would all initiate from the same point in the event loop, so could cause confusion with the sequential operation of promise.then(fnA).then(fnB) vs the ungauranteed sequence of promise.then([fnA, fnB]) - if fnA is synchronous, they are equivalent, if async, then fnB will complete before fnA (assuming fnB is synchronous or resolves faster than fnA).
I admit, this does complicate the use case of wanting multiple callbacks on the same promise, as it requires breaking the chain. var and = promise.then(fnA); and.then(fnB); and.then(fnC) to get fnB and fnC initiated after fnA.
Maybe this could be resolved with promise.and(callback, errback)? I've seen that in other implementations. Maybe supporting an array of callbacks would be easier. But in either case, I wouldn't consider it a core requirement, since it can be emulated as shown in the broken chain snippet above.
|
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 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! |
|
|
||
| results[i] = args.length > 1 ? args : args[0]; | ||
|
|
||
| if (!--remaining && allDone.getStatus() !== 'rejected') { |
There was a problem hiding this comment.
[nitpick] This conditional (on line 35) makes me very uncomfortable. I don't think it will pass shifter's jslint, either.
There was a problem hiding this comment.
I could break the decrement outside the conditional, but figured I'd save a byte or two. Will change for readability.
How dare you suggest my code would cause lint to complain ;) (it didn't, though I was using shifter --lint defaults --strict because of an issue in shifter)
|
@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 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 In reading over the API discussion linked from the Promises/A wiki entry, there was nothing specific about rationale for progress being included in |
|
@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. |
Plus when() now supports passing simple values as well as functions and promises. Previously, it would never resolve the allDone deferred if a simple value was passed.
|
I like |
* More docs in then() * errback that doesn't return a failed promise will continue to next callback, not next errback. Signals recovery from the failure. * Y.when renamed Y.batch * New Y.when(promiseOrValue, callback, errback) added * New Y.defer(callback(deferred)) added * Tests added for new APIs and changed APIs
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.