jashkenas/underscore

Wrong parameters for delay in _.debounce

kritollm opened this issue · 5 comments

delay.js

import restArguments from './restArguments.js';

// Delays a function for the given number of milliseconds, and then calls
// it with the arguments supplied.
export default restArguments(function(func, wait, args) {
  return setTimeout(function() {
    return func.apply(null, args);
  }, wait);
});

from debounce.js

 var debounced = restArguments(function(args) {
    if (timeout) clearTimeout(timeout);
    if (immediate) {
      var callNow = !timeout;
      timeout = setTimeout(later, wait);
      if (callNow) result = func.apply(this, args);
    } else {
      timeout = delay(later, wait, this, args); // Remove  this?
    }

    return result;
  });

This is the delay parameters

  export default restArguments(function(func, wait, args)

This is the passed parameters in debounce

   timeout = delay(later, wait, this, args); // Remove  this?

My theory is that the delay function has had a parameter removed without the debounce being updated, like this.

 export default restArguments(function(func, wait, context, args) {

@kritollm Thanks for reaching out. Are you reporting this as something that you noticed while reading the code, or did you actually run into problems with _.debounce?

_.restArguments is a metafunction: it takes a function (the anonymous function(func, wait, args)) and then it returns a new function (the _.delay default export). This new function accepts an arbitrary number of arguments. All the "extra" arguments are pooled together into an array that is passed as the last formal argument to the original function. So on the timeout = delay(...) line that you're asking about, the original anonymous function inside _.delay ends up being called with (later, wait, [this, args]). This is intended. Do you see how this works?

Thanks for the answer and explanation.

var later = function(context, args) { // Here (..., this, args)
    timeout = null;
    if (args) result = func.apply(context, args); // Who enjoys this result?
  };

Out of sheer curiosity. Why was this optimization never implemented? And the result, as I understand it, will only be captured the first time when you choose immediate. So what's the use case (it doesn't say anything about it in the documentation)?

I use the first version of debounce, but I needed a throttle function and looked at the source for debounce which I unfortunately did not understand

This is what I normally use.

function debounce(cb: (...args: any[]) => void, wait: number, immediate = false) {
    let timeout: number;
    return function (...args: any[]) {
        function later() {
            timeout = null;
            if (!immediate) cb(...args);
        };
        let callNow = immediate && !timeout;
        clearTimeout(timeout);
        timeout = setTimeout(later, wait);
        if (callNow) cb(...args);
    };
};

The result is always captured, but if immediate is false, it happens in the later callback.

That optimization (thanks for bringing it to my attention!) was implemented and accepted in #1269. Unfortunately, later somebody came along who thought it looked messy and undid the optimization in #2340. If you want, you can restore the optimization again and get the credit.

My questions have been answered and clarified so I close the issue.

I now also understand the reason why the result from func is always saved. Every time you run the debounced function you get the last saved result. (There are many who have wondered why #1101 and #846)

Happy to hear the question is resolved. Thanks for linking to the related issues!