FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

watch: debounce restart in watch mode by matthieusieben · Pull Request #51983 · nodejs/node · GitHub

/ node Public

watch: debounce restart in watch mode - #51983

Closed
matthieusieben wants to merge 1 commit into
nodejs:mainfrom
matthieusieben:patch-2
ConversationCommits (1)ChecksFiles changed
Closed

watch: debounce restart in watch mode#51983
matthieusieben wants to merge 1 commit into
nodejs:mainfrom
matthieusieben:patch-2

Conversation

matthieusieben commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Alternate implementation of #51971

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 6, 2024
matthieusieben mentioned this pull request Mar 6, 2024
Closed
matthieusieben force-pushed the patch-2 branch 2 times, most recently from a295353 to 77cf9d1 Compare March 6, 2024 11:20
matthieusieben mentioned this pull request Mar 6, 2024
Closed
MoLow requested changes Mar 6, 2024
View reviewed changes

MoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

node/lib/internal/watch_mode/files_watcher.js

Line 77 in bf4a63e

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

node/lib/internal/watch_mode/files_watcher.js

Line 83 in bf4a63e

this.#debouncing.add(trigger);

node/lib/internal/watch_mode/files_watcher.js

Line 86 in bf4a63e

this.#debouncing.delete(trigger);

matthieusieben mentioned this pull request Mar 6, 2024
Closed

matthieusieben commented Mar 6, 2024

Copy link
Copy Markdown
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 commented Mar 7, 2024

Copy link
Copy Markdown
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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

MoLow MoLow requested changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL