watch: add global debounce

[MoLow]

Fixes #51954

[matthieusieben]

These changes change the behavior of the class and might breaks its uses (1). They also fails to address one of the issues linked to the restart in watch mode (2).

1. This line will prevent the changed event to be triggered twice if two files are changed simultaneously.
   https://github.com/nodejs/node/pull/51986/files#diff-30ce5ad0c669a2ea6eef90f95d1f70cbf58f4b3063ff61d2498e6db30b30881bR79

2. In watch mode, the restart function will be called as many times as the changed event is triggered by the FileWatcher. Adding a globally debounced eventcthat does not affect current behaviour can be done like so main...matthieusieben:node:patch-3
   However, having the restart function called as many times as a changed occurs is sub optimal (even the way it is done in the previous patch). Indeed, imagine that several changed occurs while the application restarts. In that case, the application will be restarted as many times as the changed occurred, while only restarting once would have been enough.
   Using an iterable as in watch: debounce restart in watch mode #51983 does account for this. Using the debounced iterator will ensure that both frequent updates as well as updates happening before the next() item is requested are all bundled together, leading to fewer restarts.

[MoLow]

This line will prevent the changed event to be triggered twice if two files are changed simultaneously.

this is the goal. I am a bit confused do want debouncing or not?

Adding a globally debounced eventcthat does not affect current behaviour can be done like so

Not sure why you want to avoid effecting current behavior?

[matthieusieben]

Not sure why you want to avoid effecting current behavior?

Because of other uses of this class, like here

[MoLow]

Not sure why you want to avoid effecting current behavior?

Because of other uses of this class, like here

that is watch mode for node --test it should be fixed as well

[atlowChemi]

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)

[matthieusieben]

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.

[matthieusieben]

that is watch mode for node --test it should be fixed as well

right but that implementation depends on the owners argument, which will not contain all the owners if some events are skipped.

[matthieusieben]

Something like this might not break the current behaviour.

main...matthieusieben:node:patch-4 (#51988)

This would still cause too many restarts in case of --watch (as explained here) but it would better debounce events for the node --test case.

[MoLow]

Superseded by #51992