Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_.delay has no option to bind "this", and instead binds "this" to called function, which is useless #494

Closed
dgbeck opened this issue Feb 29, 2012 · 10 comments
Labels

Comments

@dgbeck
Copy link

@dgbeck dgbeck commented Feb 29, 2012

_.delay is incompatible with _.bind and _.bindAll.

Since _.delay calls its target function "func" with:

func.apply(func, args);

any object that was previously bound to func using _.bind or _.bindAll will not be "this" when func is called. Many other underscore functions have a special parameter for "this".. why not _.delay? Moreover, it seems totally useless to me to have "this" be a reference to the function itself. Why not just call func directly, instead of using "apply", and then not mess with the value of "this"? Am I missing something? Here is code for the _.delay function for reference purposes:

_.delay = function(func, wait) {
return setTimeout(function(){ return func.apply(func, args); }, wait);
};

@ghost
Copy link

@ghost ghost commented Feb 29, 2012

apply is needed to unpack the args array into individual arguments when invoking func...but I think the following implementation would certainly be more correct:

_.delay = function(func, wait) {
  // ...
  return setTimeout(function(){ return func.apply(this, args); }, wait);
};
@dgbeck
Copy link
Author

@dgbeck dgbeck commented Feb 29, 2012

Yes, I think that would be more consistent with the rest of the library, and more useful.

Thanks

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 2, 2012

We can make that change -- but how would that possibly allow the value of this to be bound? Perhaps provide a test case?

@ghost
Copy link

@ghost ghost commented Apr 2, 2012

Erm...whoops. A third context argument would need to be added; my suggestion above isn't useful at all.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 2, 2012

In that case, I'm going to change it to null.

@jashkenas
Copy link
Owner

@jashkenas jashkenas commented Apr 2, 2012

Fixed in: 1366d90

@jashkenas jashkenas closed this Apr 2, 2012
@machineghost
Copy link

@machineghost machineghost commented Mar 14, 2013

Quick note for any future readers who are still unsatisfied with:

_(
    _(function() {
        this.doSomething();
   }).bind(this)
).delay(500);

There are several alternative options you can use:

  1. Use the chain method (without needing to use the accompanying value method):
_(function() {
    this.doSomething();
}).chain().bind(this).delay(500);
  1. Mix-in your own boundDelay method so that you can do:
    _(function() {
        this.doSomething()
    }).boundDelay(this, 500);

That would look something like this (untested) code:

_.mixin({
    boundDelay: function(func, context, delayMs) {
        var boundVersion = _(func).bind(context);
        _(boundVersion).delay(delayMs);
    }
});
  1. You can Underscore Grease (https://github.com/machineghost/Underscore-Grease) to chain the two in to:
    _(function() {
        this.doSomething();
   }).bind_(this).delay(500);
@jeff-h
Copy link

@jeff-h jeff-h commented Apr 23, 2015

I don't understand how we can call this "fixed". The various solutions in the comment above all appear very unreadable to me.

The change in 1366d90 simply stops sending an odd context to the called function.

Is there a reason delay can't take a context argument as per #494 (comment) ? That, to me, would be awesome :)

@jridgewell
Copy link
Collaborator

@jridgewell jridgewell commented Apr 23, 2015

There's no reason to change _.delay. If you want a context, bind it beforehand.

_.delay(_.bind(func, context), 500)
@jeff-h
Copy link

@jeff-h jeff-h commented Apr 23, 2015

Thanks! I should have tried that I guess, but assumed wrongly based on the complexity of the above answers that something more complicated was needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.