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: ensure watch mode detects deleted and re-added files #52879

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

znewsham
Copy link
Contributor

@znewsham znewsham commented May 7, 2024

In systems that don't support recursive file watching, If you watch a file that gets replaced as sometimes happens (e.g., gedit and certain configurations of docker), subsequent changes wont be watched.

This PR detects rename events and adds a 60s timeout with a 1s interval to watch for the path to exist, then emits a change event for that file, ensuring that:

  1. if the contents of the file changed between being removed and re-added that change is detected
  2. subsequent changes are detected

Split out from #50890

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 7, 2024
@RedYetiDev RedYetiDev added fs Issues and PRs related to the fs subsystem / file system. watch-mode Issues and PRs related to watch mode and removed fs Issues and PRs related to the fs subsystem / file system. labels May 7, 2024
@MoLow
Copy link
Member

MoLow commented May 8, 2024

@znewsham
Copy link
Contributor Author

znewsham commented May 8, 2024

I'm not familiar with that, are you suggesting that the change be made to the general recursive watch mechanism instead of the watch/restart functionality? Currently, on linux - the watch functionality isn't recursive at all, so I'm not sure how changes to the recursive mechanism would change the behaviour in watch mode?

@MoLow
Copy link
Member

MoLow commented May 8, 2024

I'm not familiar with that, are you suggesting that the change be made to the general recursive watch mechanism instead of the watch/restart functionality? Currently, on linux - the watch functionality isn't recursive at all, so I'm not sure how changes to the recursive mechanism would change the behaviour in watch mode?

Yes, that is my suggestion. that file is an implementation for environments where recursive watch isn't natively implemented via libuv, and it was added after the addition of watch mode.

my guess that file already handles this edge case, and if not it can be added there

I started working on usage of non native recursive watch at #45271 and there were some issues (in the CI if I recall), but I would be happy if you or someone else picked it up

@znewsham
Copy link
Contributor Author

znewsham commented May 9, 2024

what's the advantage of a recursive watch? Particularly in environments where it isn't natively supported, where it's still one watcher per file.

The nice thing about non recursive watch mode is it should have near perfect detection - no false positives where an unused file triggers a restart. It's possible I've missunderstood how the recursive watch works though.

A trivial limitation of the recursive watch implementation you linked is that it appears it doesn't follow symlinks, so changes to local NPM modules (installed via symlink) won't trigger a restart.

My goal with this PR is not that it implements (or fixes) recursive file watching, but that in non recursive mode it correctly detects changes to a single watched file

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. watch-mode Issues and PRs related to watch mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants