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

bug: autoWatchBatchDelay option is being "ignored" #1520

Closed
ferrarit opened this issue Jul 27, 2015 · 11 comments · May be fixed by Omrisnyk/npm-lockfiles#122 or Omrisnyk/npm-lockfiles#132
Closed

bug: autoWatchBatchDelay option is being "ignored" #1520

ferrarit opened this issue Jul 27, 2015 · 11 comments · May be fixed by Omrisnyk/npm-lockfiles#122 or Omrisnyk/npm-lockfiles#132

Comments

@ferrarit
Copy link

autoWatchBatchDelay options is irrelevant in current version of karma

The problem seems to be caused by the use of _.throttle

fabdbae#diff-e8e0621a959fd2639bcc23cd2601f67fR73

replacing _.throttle with _.debounce fixes the issue

this._emitModified = _.debounce(function () {
    self._emitter.emit('file_list_modified', self.files)
}, this._batchInterval)
@dignifiedquire
Copy link
Member

I can't believe I did this mistake again, for some reason I'm never able to use throttle and debounce correctly.
Thanks a lot for reporting, would you be up for a PR?

@ferrarit
Copy link
Author

Honestly, I wouldn't know how to write the tests :)

@dignifiedquire
Copy link
Member

@ferrarit there are actually tests https://github.com/karma-runner/karma/blob/master/test/unit/file-list.spec.coffee#L568-L639 which start failing when I introduce debounce. Could you maybe create a small setup that reproduces the behaviour you are seeing?

@ferrarit
Copy link
Author

I have created a simple test here: https://github.com/ferrarit/karma-test

  1. npm install
  2. karma start karma.conf
  3. edit both test/test-a.js and test/test-b.js
  4. save both files at the same time
  5. You should get a message like "Delaying execution, these browsers are not ready..." and tests are going to run twice

  1. Replace _.throttle with _.debounce
  2. edit both test/test-a.js and test/test-b.js
  3. save both files at the same time
  4. tests only run once

@dignifiedquire
Copy link
Member

Thanks :octocat:

@ferrarit
Copy link
Author

Thank you sir for the quick fix and the amazing job you are doing with Karma. 👍

@dignifiedquire
Copy link
Member

Looks like my fix is not working yet :( Will revisit tomorrow with a fresh brain

@Merott
Copy link
Contributor

Merott commented Aug 2, 2015

Any update on this?

I have no experience in using either throttle or debounce, but I was wondering, whichever is used, shouldn't the leading option be set to false? it seems that throttle is more of a convenience function anyway, as it forwards the call to debounce with some default options.

I've had no luck trying to fix this issue myself.


EDIT

I managed to fix the issue with this commit. It sets leading to false, and also ensures that the first time that _emitModified is called, it emits the event without delay, by making it a throttled function only after the first call. It seems that otherwise some initialisations (e.g. this) won't happen and lead to exceptions.

Any thoughts?

@dignifiedquire
Copy link
Member

Released in v0.13.5

@Merott
Copy link
Contributor

Merott commented Aug 4, 2015

Although this fixes the issue for short delays, If you use a longer delay (e.g. 1000), you'll get a TypeError: Cannot read property 'then' of undefined error at common.js#12. It happens because the code in common.js gets hit before the promise is initialised inside of web-server.js#39.

That was why I suggested throttling the event after its first emission.

@dignifiedquire
Copy link
Member

Thanks @Merott for pointing this out, this should be fixed in v0.13.6

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

Successfully merging a pull request may close this issue.

3 participants