can.debounce keeps the context after being called the first time #782

Closed
colinexl opened this Issue Mar 12, 2014 · 4 comments

Comments

Projects
None yet
4 participants
@colinexl

Hello,

We believe there is a bug in the way can.debounce works:

function (fn, time, context) {
  var timeout;
  return function () {
    var args = arguments;
    context = context || this; // buggy line, can be fixed by using var myContext = context || this
    clearTimeout(timeout);
    timeout = setTimeout(function () {
      fn.apply(context, args);
    }, time);
  };
}

The context is saved in the returned function's closure so that all future calls to the debounce will get the saved context.

We discovered this bug when using can.debounce to create a event handler function for a can.Component. Once the can.Component is destroyed and reinitialized, the old (destroyed) can.Component's context is used in the can.debounce function.

I can create a JSFiddle if required but this is pretty easy to reproduce.

Thanks,
Colin Zhu

@daffl daffl added the Bug label Mar 19, 2014

@daffl daffl added this to the 2.1.0 milestone Mar 19, 2014

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 19, 2014

Contributor

Makes sense. The easiest way would probably be to change it to:

function (fn, time, context) {
  var timeout;
  return function () {
    var args = arguments;
    clearTimeout(timeout);
    timeout = setTimeout(can.proxy(function () {
      fn.apply(this, args);
    }, context || this), time);
  };
}
Contributor

daffl commented Mar 19, 2014

Makes sense. The easiest way would probably be to change it to:

function (fn, time, context) {
  var timeout;
  return function () {
    var args = arguments;
    clearTimeout(timeout);
    timeout = setTimeout(can.proxy(function () {
      fn.apply(this, args);
    }, context || this), time);
  };
}
@colinexl

This comment has been minimized.

Show comment
Hide comment
@colinexl

colinexl Mar 21, 2014

Yep I think that will solve the issue.

Yep I think that will solve the issue.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 3, 2014

Contributor

@daffl or @colinexl anyway one of you can get a pull request / test in by tomorrow? Otherwise, we might save this fix for 2.1.1.

Contributor

justinbmeyer commented May 3, 2014

@daffl or @colinexl anyway one of you can get a pull request / test in by tomorrow? Otherwise, we might save this fix for 2.1.1.

@justinbmeyer justinbmeyer modified the milestones: 2.2.0, 2.1.0, 2.1.1 May 3, 2014

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings May 20, 2014

Contributor

@colinexl Can you include your use case in this thread

Contributor

ccummings commented May 20, 2014

@colinexl Can you include your use case in this thread

@ccummings ccummings modified the milestones: 2.1.2, 2.1.1 May 22, 2014

daffl added a commit that referenced this issue Jun 11, 2014

@daffl daffl closed this in #1073 Jun 11, 2014

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