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

debounce timing #142

Open
wants to merge 175 commits into
base: master
Choose a base branch
from
Open

debounce timing #142

wants to merge 175 commits into from

Conversation

motemen
Copy link

@motemen motemen commented Jul 31, 2014

Hi, I encountered a problem that when a file under watch is modified many times successively in a short period (e.g. building a file by appending), there may be a situation that the "changed" event is not emitted after the last modification ocurred. So the callback function cannot see the "final content" of the file.

This seems because that when an event ocurred, next events are blocked for debounceDelayms. So I added a code trying to emit the event after the last filesystem event, even if emitting was blocked by previous event's delay.

I wonder if this is a good solution, but I'm glad if you take a look at it. Thanks.

shama added 30 commits March 19, 2014 21:14
These tests were for specific cases handled by private methods
that have now been removed and therefore these tests should be removed.
@shama
Copy link
Owner

shama commented Jul 31, 2014

Thanks for the PR! I'll look into this shortly.

@expectlabs-andrew
Copy link

Just wondering if there is a plan to address this issue. Sounds like a pretty serious issue if an event is not issued immediately after the last change. It does seem like a shame not to just use lodash/underscore debounce but I understand if you want to avoid extra dependencies. Thanks.

@shama
Copy link
Owner

shama commented Apr 17, 2015

@expectlabs-andrew I haven't had time to address this. Currently it fails test (and now has a merge conflict).

I don't think this issue is that serious though. It already does debounce just not the exact time the OP wants. FWIW, I'd prefer to not do this internally and let users choose how/if they want to debounce.

So this is not a high priority for me but if someone else wanted to fix this to not fail tests I'd be happy to merge.

@expectlabs-andrew
Copy link

Thanks for the update. Debouncing locally is by fine, but this worried me enough to go back to gulp.watch (where I can also use lodash to debounce). My concern is just that the out-of-the-box default behavior can actually leave unhandled events on the table. If that is, in fact, the default behavior, then it might be just be better to disable debouncing by default. To debounce locally, my reading of the code is that I explicitly need to set debounceDelay to 0 instead of the default value of 500.

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

Successfully merging this pull request may close these issues.

None yet

10 participants