adding standard node callback support #2

Open
wants to merge 1 commit into
from

Projects

None yet

2 participants

@thesmart

adding method "thenNode" which takes a standard node callback (e.g. function(err, result))

Take a look and let me know what you think.

@thesmart thesmart updating test to work in Node as of 0.8.15
adding method "thenNode" which takes a standard node callback (e.g. function(err, result))
275381a
@MaxMotovilov
Owner

Not sure I like opt_scope -- none of the other methods in the API has the extra auto-binding parameter and, arguably, p.thenNode( mycb.bind( myobj ) ) is more transparent to a reader than p.thenNode( mycb, myobj ) even if slightly longer. Counterarguments?

Also, by design of the API, promise.when(p,...) is a more universal form of p.then(...) (i.e. it works in an obvious way with non-promise arguments). In order to extend this to the other callback type, we'd also have to add a whenNode() API.

Finally, your proposed API does not support the third argument of then(), the progressCallback.

I suspect that it would be easier to overcome these objections if, instead of creating an entirely different method for node-style callbacks, we have somehow determined that an argument passed into then() (or when()) is a node-style callback and thus should take place of both callback and errback. So, how about this:

function _Pair( cb, eb ) {
  this.callback = cb;
  this.errback = eb;
}

exports.nodeCallback = function( cb, opt_scope ) {
   return new _Pair(
      cb.bind( opt_scope || null, null ),
      cb.bind( opt_scope || null )
   );
}

and then have then() check for instanceof _Pair and branch accordingly?

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