watch should debounce the restart

[matthieusieben]

What is the problem this feature will solve?

When watching the result of a build, and a build is triggered, node --watch will take a long time to actually restart. This is very likely due to the fact that it will trigger a restart for every single file that is changed.

What is the feature you are proposing to solve the problem?

Waiting a (small) amount of time to allow more changes to occur before actually stopping & re-starting would make this feature much smoother when running in dev mode.

What alternatives have you considered?

Status quo implies that the restart is slow, even when small changes are made, depending on the build system.

For example, when working on a large tsc --watch, typescript will output a new file for any edited files, as well as all of its dependent. In this setup (or when the dev triggers a new full build of is ts project), node (in watch mode) take quite long to actually restart, due to the overhead of starting & stopping for every file that changes.

[MoLow]

watch mode is already debounced:

node/lib/internal/main/watch_mode.js

Line 47 in b34512e

const watcher = new FilesWatcher({ debounce: 200, mode: kShouldFilterModules ? 'filter' : 'all' });

[matthieusieben]

Indeed, but that debounce option seems to act on individual file path. Not on multiple files.

node/lib/internal/watch_mode/files_watcher.js

Lines 76 to 89 in bf4a63e

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

Try starting a project in --watch mode and run the following (from a place that contains multiple files being currently run)

for f in $(find . -name '*.js' -print); do
touch "$f"; sleep 0.05;
done

You will notice the following output from node --watch:

I believe the solution would be to debounce the calls to restart here:

node/lib/internal/main/watch_mode.js

Lines 120 to 122 in b34512e

	for await (const _ of on(watcher, 'changed')) {
	await restart();
	}

[matthieusieben]

See #51971 and #51983 for possible solutions.

[MoLow]

@matthieusieben what do you think about such a solution? main...MoLow:watch-mode-global-debounce

diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js
index bbc13e67cc..02c836e14a 100644
--- a/lib/internal/watch_mode/files_watcher.js
+++ b/lib/internal/watch_mode/files_watcher.js
@@ -6,6 +6,7 @@ const {
   SafeMap,
   SafeSet,
   StringPrototypeStartsWith,
+  Symbol,
 } = primordials;
 
 const { validateNumber, validateOneOf } = require('internal/validators');
@@ -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();
@@ -74,16 +76,17 @@ class FilesWatcher extends EventEmitter {
   }
 
   #onChange(trigger) {
-    if (this.#debouncing.has(trigger)) {
+    if (this.#debouncing.has(trigger) || this.#debouncing.has(kGlobalDebounce)) {
       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();
   }

[matthieusieben]

Hey @MoLow, I think that solution looks much simpler. It does change the way the class works though. An event will no longer be emitter for every individual change, which will break other current uses of that class.

[matthieusieben]

What about something like this then ?

main...matthieusieben:node:patch-3

[matthieusieben]

Recap of the things i tried:

• main...matthieusieben:node:patch-1 (watch: debounce restart in watch mode #51971)
  Work great (actually debounces and ignores repeated events while restarting). Implementation is a bit "weird" because of the promise that needs to be awaited.
• main...matthieusieben:node:patch-2 (watch: debounce restart in watch mode #51983)
  Functionnaly identical to previous. Code is more complex but cleaner an more re-usable (IMHO).
• main...matthieusieben:node:patch-3
  This adds a globally debounced event. This behavior still causes too many restarts (unlike patch-1 and patch-2)
• main...matthieusieben:node:patch-4 (watch: Debounce all files updates into a single event. #51988)
  This debounces all events (similar to previous) without adding a new event. This behavior still causes too many restarts (unlike patch-1 and patch-2)

#51986 suffers the same issue as patch-3