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.

Sign up for GitHub

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

watch: add global debounce #51986

Closed
wants to merge 1 commit into from
Closed

watch: add global debounce #51986

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/internal/watch_mode/files_watcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const {
SafeMap,
SafeSet,
StringPrototypeStartsWith,
Symbol,
} = primordials;

const { validateNumber, validateOneOf } = require('internal/validators');
Expand All @@ -21,6 +22,7 @@ const { setTimeout } = require('timers');
const supportsRecursiveWatching = process.platform === 'win32' ||
process.platform === 'darwin';

const kGlobalDebounce = Symbol('kGlobalDebounce');
class FilesWatcher extends EventEmitter {
#watchers = new SafeMap();
#filteredFiles = new SafeSet();
Expand Down Expand Up @@ -74,16 +76,17 @@ class FilesWatcher extends EventEmitter {
}

#onChange(trigger) {
if (this.#debouncing.has(trigger)) {
if (this.#debouncing.has(trigger) || this.#debouncing.has(kGlobalDebounce)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We always set the 2 of these together, can't we simplify by only setting & checking for the kGlobalDebounce? (and drop the .add(trigger) as well)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Piggy backing on this comment to emphasis that indeed, with this implementation, the second part of the if will always be true if any other file already triggered as #onChange. This not only renders the use of the this.#debouncing set unnecessary (a simple boolean would suffice), but it also means that the owners argument of the event will no longer contain all the owners. Indeed, a change to file A would cause kGlobalDebounce to be set so that any change to file B would get ignored by this if condition.

return;
}
if (this.#mode === 'filter' && !this.#filteredFiles.has(trigger)) {
return;
}
this.#debouncing.add(trigger);
this.#debouncing.add(trigger).add(kGlobalDebounce);
const owners = this.#depencencyOwners.get(trigger);
setTimeout(() => {
this.#debouncing.delete(trigger);
this.#debouncing.delete(kGlobalDebounce);
this.emit('changed', { owners });
}, this.#debounce).unref();
}
Expand Down