Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

add a completely backwards compatible way to use async.apply with a context via call #209

Closed
wants to merge 4 commits into from

4 participants

@azylman

Basically, what this does is make it so that, in async.apply, if the value of "this" is anything but async (which would only happen if you specified a thisArg via something like call or apply), it passes that as the thisArg to fn.apply instead of undefined.

See the test cases I've added (shamelessly ripped out of #81) for usage.

@azylman

This is a bit awkward with the async.apply.call syntax, but at least now there is SOME way to do this, instead of no way. Also, it should be completely backwards compatibly.

@caolan
Owner

An interesting approach. I think I'd prefer adding a new function called 'bind' or something to make it clear what's going on: async.bind(myfn, myobj, [args]) ...it's a shame that async.apply doesn't use an array for arguments really, that would have made things much more simple. Perhaps we can deprecate async.apply in favour of async.partial and async.bind which would essentially mirror the underscore API? Then keep async.apply around for backwards compatibility but remove it from the README...

@azylman

I like the idea of a new function, but in the mean time I think that this seems pretty explicit to me. You have to go out of your way to do async.apply.call and pass in a value for "this" and if you're just doing "async.apply" the behavior will stay the same as what you're used to.

I think in some sense you could almost think of this like a bugfix since, previously, if you tried to use any of the call or apply methods that are part of Function objects to change the value of 'this', async.apply would ignore them and still use null.

@brianmaissy

Haha now that we've closed all the duplicates, let's get to actually solving this.

I like @caolan's API change suggestion, but maybe people should just be using underscore instead? Is there a good reason to exactly duplicate functionality which exists elsewhere and doesn't really have anything essentially asynchronous about it?

Also, what about the issue of calling map or the other collection functions with a context? Should that be done by binding the iterator to a context manually before passing it to async, and then respecting it there? I'm worried that doing so could lead to a lot of unexpected errors. What I sort of like about async's current behavior, despite it's lack of expressiveness, is that it's always predictable. You have no context (other than the global context) when executing iterators, and if you try writing an iterator as if you did, you get errors. For example, like in issue #46, you could imagine someone using an iterator from an external library that looks up this.fooBar, where fooBar is a relatively common attribute name, and getting a garbage value instead of an error. [Edit: this is wrong. See below]

@azylman

Also, what about the issue of calling map or the other collection functions with a context? Should that be done by binding the iterator to a context manually before passing it to async, and then respecting it there? I'm worried that doing so could lead to a lot of unexpected errors. What I sort of like about async's current behavior, despite it's lack of expressiveness, is that it's always predictable. You have no context (other than the global context) when executing iterators.

I was looking into it a little bit and it looks like bind takes precedence over whatever this parameter is specified with apply or call, e.g.:

var x = 9; 
var module = {
  x: 81,
  getX: function() { return this.x; }
};

module.getX(); // 81

var getX = module.getX;
getX(); // 9, because in this case, "this" refers to the global object

// create a new function with 'this' bound to module
var boundGetX = getX.bind(module);
boundGetX(); // 81
boundGetX.apply(null); // 81
boundGetX.call(null); // 81

If that's the case, I don't think any changes would need to be made to async for map etc. to support context on iterators. Using the example from the issue you linked to, async.concat(keys, client.hgetall, myCallback); would just need to be async.concat(keys, client.hgetall.bind(client), myCallback); and it should just work.

In that case, I don't think you need to worry about predictability since the only time you have any context is when you explicitly pass in a function that is the result of bind, where you almost definitely wanted an explicit context anyway (since you're explicitly creating one).

@brianmaissy

Thank you! You are completely correct; I had a mistaken understanding of the way bind works.

I think my question about whether or not async needs its own bind and partial still stands.

@azylman

I believe that the problems with async.apply can also be solved with bind - e.g. async.apply(module.getX.bind(module))(); so it's probably not worth spending a lot of time on - either get rid of it or keep it around, but either way it might be useful to add something to the documentation showcasing that you can use bind to solve this problem, since there's been more than one person who has had issues with it - perhaps adding an example that uses bind to the "Quick Examples" function.

@brianmaissy

it might be useful to add something to the documentation showcasing that you can use bind to solve this problem

Great idea. I'll write one up. #255

@azylman

The new README additions look good. Unless there's any objection, I feel like this is resolved and I'm going to close it.

@brianmaissy

Sounds good :thumbsup: Good work team!

@azylman azylman closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 11, 2012
  1. @azylman

    fix async.apply

    azylman authored
  2. @azylman

    derp

    azylman authored
  3. @azylman

    add test cases

    azylman authored
  4. @azylman

    small fix

    azylman authored
This page is out of date. Refresh to see the latest.
Showing with 26 additions and 2 deletions.
  1. +2 −1  lib/async.js
  2. +24 −1 test/test-async.js
View
3  lib/async.js
@@ -521,9 +521,10 @@
async.apply = function (fn) {
var args = Array.prototype.slice.call(arguments, 1);
+ var context = this === async ? null : this;
return function () {
return fn.apply(
- null, args.concat(Array.prototype.slice.call(arguments))
+ context, args.concat(Array.prototype.slice.call(arguments))
);
};
};
View
25 test/test-async.js
@@ -986,7 +986,7 @@ exports['sortBy'] = function(test){
};
exports['apply'] = function(test){
- test.expect(6);
+ test.expect(21);
var fn = function(){
test.same(Array.prototype.slice.call(arguments), [1,2,3,4])
};
@@ -995,6 +995,29 @@ exports['apply'] = function(test){
async.apply(fn, 1, 2)(3, 4);
async.apply(fn, 1)(2, 3, 4);
async.apply(fn)(1, 2, 3, 4);
+
+ var thisArg = {
+ fn: function() {
+ test.same(Array.prototype.slice.call(arguments), [1, 2, 3, 4]);
+ test.equal(this, thisArg);
+ }
+ };
+ async.apply.call(thisArg, thisArg.fn, 1, 2, 3, 4)();
+ async.apply.call(thisArg, thisArg.fn, 1, 2, 3)(4);
+ async.apply.call(thisArg, thisArg.fn, 1, 2)(3, 4);
+ async.apply.call(thisArg, thisArg.fn, 1)(2, 3, 4);
+ async.apply.call(thisArg, thisArg.fn)(1, 2, 3, 4);
+ var objFn = {
+ apply: function() {
+ fn.apply.apply(fn, Array.prototype.slice.call(arguments));
+ }
+ };
+ async.apply(objFn, 1, 2, 3, 4)();
+ async.apply(objFn, 1, 2, 3)(4);
+ async.apply(objFn, 1, 2)(3, 4);
+ async.apply(objFn, 1)(2, 3, 4);
+ async.apply(objFn)(1, 2, 3, 4);
+
test.equals(
async.apply(function(name){return 'hello ' + name}, 'world')(),
'hello world'
Something went wrong with that request. Please try again.