callbacks.{call,apply,bind,promisify} #93

Merged
merged 28 commits into from Feb 6, 2013

Projects

None yet

2 participants

@renato-zannon

(final?) part of #73.

I've implemented the functions as we discussed on the above issue, including the object-based API for promisify. I'm not fully satisfied by the implementation (maybe there's a way to make it clearer that I'm not seeing?), but I think it should be enough for a conversation starter.

One thing that we haven't directly discussed is whether promisify should have the ability to partially apply arguments, as bind does. I haven't included that ability for now, but should be easy to do.

Also, on a slightly less technical issue, I have no idea of how to write the header of the file (the one with the licensing and all that) :) Should I just copy it from one of the other files?

@briancavalier
The Javascript Architectural Toolkit member

Can this be hoisted out of promisify?

@briancavalier
The Javascript Architectural Toolkit member

Hmmm, this reverses the order of the args, is that intentional?

It is. The arguments added to endArgs are the ones with negative index, and are added in a 'offset from the end' order 101a23d#L0R149

The Javascript Architectural Toolkit member

ah, thanks for clarifying!

Is this too cryptic? Maybe it deserves a comment, or to be done in a more obvious way. What do you think?

The Javascript Architectural Toolkit member

+1 for comments. I think that'll be sufficient for now.

Done! Check out renato-zannon/when@26032f5 to see if it is clear enough

The Javascript Architectural Toolkit member

comments look good, thanks!

@briancavalier
The Javascript Architectural Toolkit member

Clever!

@briancavalier
The Javascript Architectural Toolkit member

I'll take a closer look later today. Your comment about switching the module from being a function to an object has me thinking now. I'm actually a fan of modules that are functions (and decorated functions, as when itself is). What are your thoughts on having the callbacks, function, and node/function be functions ... perhaps each would be a function equivalent to its call() method? Hmmm, I'm not sure. I'll give it some thought, but def would like to hear what you think.

@renato-zannon

I'm a fan myself :) But I think it isn't a good fit for this case, because we would then have the call, apply and/or bind methods of a function to be completely different from what people would expect. I think it might be very confusing :)

@briancavalier
The Javascript Architectural Toolkit member

Duh, of course, you are right about clobbering the default call, apply, and bind :) We have to stick with an object if we keep those function names.

On another note, I've become quite fond of the name try as a possible synonym for call. It can't be used in environments that disallow keyword property names, but otherwise, it's a good word.

Anyway, this is looking good from my perspective. Anything else you feel we need to do before merging?

@renato-zannon

I think we have two things that need to be sorted out before:

  1. Should promisify be a proper superset of bind, by accepting partial application of the leftmost arguments?
  2. Should the functions reject the promise on the case of a synchronous exception?
@briancavalier
The Javascript Architectural Toolkit member

Thanks, excellent questions. Here are some thoughts:

  1. On one hand, I like the "proper superset" notion. On the other, Function.prototype.bind() would work just fine on the function returned by promisify. I'm torn. My initial inclination is not to allow partial application, and if people ask for it, add it. What are your thoughts on allowing partial application?

  2. Yes, absolutely :)

And here is another question:

  1. Should we automatically resolve function arguments? i.e. allow promises as arguments, and do the necessary magic (when.all) to resolve them before invoking the ultimate function. This sounds interesting, but may be overkill? Or perhaps this should be separate functionality that could be combined with call/apply/bind?

This is somewhat related to your 2, since using when.all() to resolve the arguments would safely nest the target function call inside a promise callback, automatically guaranteeing a rejection if the target method throws synchronously.

Thoughts?

@renato-zannon
  1. Agreed :) I think promisify already has a pretty large API, and we would end up implementing too much if we wanted to cover every possible use case. I think it is flexible enough right now :)

  2. Will do then!

  3. I think it makes sense to have that baked in, since there's a large portion of the library that does it :) And, from my experience as a user, it's good to get everything normalized and not have to worry that much about what's passed in.

Oh, and there's another small thing: About the copyright notice on the top of the file, I just need to copy one from other file, or there's something special I should do? :)

@briancavalier
The Javascript Architectural Toolkit member

Oops, sorry I forgot to answer your copyright question before. Yes, you can copy and paste it. Please do list yourself as a @contributor in a top-level comment (unless you'd prefer not to, it's entirely your choice).

  1. Cool, let's go without partial application for now.
  2. See next :)
  3. Yeah, I'm coming around to believing that as well. To play devil's advocate for a minute, though, it's really easy for someone to do when.join(a, b, c).spread(myFunction); or when.all(args).spread(myFunction);, which is effectively the same thing. Hmmm, now that I write that, maybe when/function's apply() should literally be return when.all(slice.call(arguments, 1)).spread(myFunction);?
@renato-zannon

Done! Now there's one last place that doesn't account for promises-as-arguments: the 'bind` variants, on the partial application arguments. Do you think there's value in accounting for promises on these too? I can think of a scenario, though I don't know if it would be common enough to warrant support by the library:

var sharedParametersRequest = when($.getJSON("/parameters.json"));
var myFunction = fn.bind(functionThatUsesTheParameters, sharedParametersRequest);

// These two calls share the same 'parameters request', which, once finished, will enable both
// of them to be executed.
myFunction("/foo").then(/* do stuff */);
myFunction("/bar").then(/* do moar stuff*/);

About your 3rd point, it's true that many of these functions are simple to write inline, but I think there's a value in having these common use cases optimized. One or two times is okay, but when these patterns start to crop up throughout the code, it makes them wish to have way with less friction. That's what made me open the PR in the first place :)

@briancavalier
The Javascript Architectural Toolkit member

Hmmm, it seems like bind() may already work since it delegates to apply? If not, then it shouldn't be too hard to allow it. It seems like if we're going to allow promise args, we should just do it across the board.

I completely agree about optimizing common use cases. I don't know if we have enough real evidence yet to know for sure that resolving promise arguments will be a common use case, but as you said, it sure feels like the right thing to do :) Let's do it.

@renato-zannon

Turns out that I was wrong :) I've explicitly tested for the partial application using promises, and it works just fine.

Fair point! But even without the evidence, I think that the argument of consistency of the library is a strong one. Also I don't see any case on which the automatic resolution would be a problem... So, even if it doesn't help that much, I think it won't be an issue for anyone :)

@briancavalier
The Javascript Architectural Toolkit member

nice clarification :)

@briancavalier
The Javascript Architectural Toolkit member

Yep, we're in agreement. The only cases I've thought of (and I feel like I'm grasping at straws here) are:

  1. Someone actually wants to receive the promises as arguments, rather than their values. I'm fine with simply saying that that person simply shouldn't use call/apply/bind and should find another way to accomplish their goal.
  2. Performance. We're introducing an extra when.all to resolve the arguments. I doubt this will be an issue, but I also see it as motivation for us to continue to make sure when.js is as fast as it can possibly be :)
@briancavalier
The Javascript Architectural Toolkit member

Seems like we've sorted out the 3 issues from above. Is there anything else you'd like to tweak?

@renato-zannon

I think it is okay now :) I don't have any more issues or improvements in mind.

@briancavalier briancavalier merged commit dc50432 into cujojs:dev Feb 6, 2013

1 check passed

Details default The Travis build passed
@briancavalier
The Javascript Architectural Toolkit member

Thank you for the awesome work on this stuff!

@renato-zannon

My pleasure :)

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