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

watch: debounce restart in watch mode #51983

Closed
wants to merge 1 commit into from

Conversation

matthieusieben
Copy link
Contributor

Alternate implementation of #51971

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 6, 2024
@matthieusieben matthieusieben force-pushed the patch-2 branch 2 times, most recently from a295353 to 77cf9d1 Compare March 6, 2024 11:20
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

FilesWatcher already has debounce internally, a much simpler solution would be to change / add a debounce key to something global instead of the file name:

if (this.#debouncing.has(trigger)) {

this.#debouncing.add(trigger);

this.#debouncing.delete(trigger);

@matthieusieben
Copy link
Contributor Author

I agree, and I actually tried that already (main...matthieusieben:node:patch-4)

However, as explained in the original issue, this implementation does more than fixing the debounce. It also prevents the app from restarting too many times.

@matthieusieben
Copy link
Contributor Author

This solution is not only more complex, it also adds async ticks to the processing of the event, which might cause un-necessary restarts. #51992 is the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants