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

limit dev watcher to avoid overhead from third party processes writing to root #8341

Open
4 tasks done
dominikg opened this issue May 26, 2022 · 4 comments
Open
4 tasks done
Labels
enhancement New feature or request performance Performance related enhancement

Comments

@dominikg
Copy link
Contributor

Clear and concise description of the problem

By default, vite devuses a chokidar watcher on root, ignoring only .git and node_modules.

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/index.ts#L291-L302

This causes overhead when processes write files to root that are not needed to be watched. eg. @playwright/test writes to test-results - a lot - if tracing is enabled. (see details below)

Suggested solution

Limit watching to directories matching the fs.allow pattern inside root. This gives frameworks and users more control over what is watched and improves performance.

example code, not perfect but to illustrate whats suggested.

  const { ignored = [], ...watchOptions } = serverConfig.watch || {}
-  const watcher = chokidar.watch(path.resolve(root), {
+  const scope = path.resolve(root) + path.sep;
+  const watch = serverConfig.fs?.allow?.length ? serverConfig.fs.allow.filter(dir => dir.startsWith(scope)) : [];
+  if(watch.length === 0) {
+    watch.push(path.resolve(root))
+  }  
+  const watcher = chokidar.watch(watch, {    
    ignored: [
      '**/node_modules/**',
      '**/.git/**',
      ...(Array.isArray(ignored) ? ignored : [ignored])
    ],
    ignoreInitial: true,
    ignorePermissionErrors: true,
    disableGlobbing: true,
    ...watchOptions
  }) as FSWatcher

Considerations:

  1. fs.allow can contain items outside root. simply watching all fs.allow entries could cause even more problems (what if someone put / in fs.allow?) so keep the root boundary in place.
  2. keep the current behavior if fs.allow contains root or only contains directories outside root (it has to watch something).
  3. could use some logging too what gets watched and maybe a warning on excessive flooding

Alternative

manually add ignore entries to known patterns.

watch: {
	// perf, do not watch playwright output dir
	ignored: ['**/test-results/**']
}

This would still incur some cost inside chokidar but at least it would not reach the downstream handlers

Additional context

example of a sveltekit testsuite run with enabled logging inside a watcher demonstrating the current behavior.
https://github.com/sveltejs/kit/runs/6599185996?check_suite_focus=true

...
@sveltejs/kit:test: [WebServer] config.kit.files.routes: D:\a\kit\kit\packages\kit\test\apps\basics\src\routes
@sveltejs/kit:test: [WebServer] add D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-0\41adcbc97f1df5cc5fc3997a22928727.zip
@sveltejs/kit:test: [WebServer] add D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-0\f845da79f5fc1bf257a216cc3e2eb100.png
@sveltejs/kit:test: [WebServer] unlink D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-0\41adcbc97f1df5cc5fc3997a22928727.zip
@sveltejs/kit:test: [WebServer] unlink D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-0\f845da79f5fc1bf257a216cc3e2eb100.png
@sveltejs/kit:test: [WebServer] add D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-1\ec2416cbd7c50f61810b6fac2a86d166.zip
@sveltejs/kit:test: [WebServer] add D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-1\639ae30030d1658af3b1a90aae33d8fc.png
@sveltejs/kit:test: [WebServer] unlink D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-1\ec2416cbd7c50f61810b6fac2a86d166.zip
@sveltejs/kit:test: unlink D:\a\kit\kit\packages\kit\test\apps\basics\test-results\.playwright-artifacts-1\639ae30030d1658af3b1a90aae33d8fc.png
...

Validations

@dominikg
Copy link
Contributor Author

Additional examples of directories never needed to be watched:
ide/editor settings: .idea, .vscode ...

@bluwy
Copy link
Member

bluwy commented Jun 13, 2022

Thinking about it more, I think the alternative would be the way forward though. There could be usecases where files should be watched but isn't covered in fs.allow (maybe a plugin that derives data from files).

This would still incur some cost inside chokidar but at least it would not reach the downstream handlers

I'm not sure how watch.ignored would incur a cost. Checking chokidar's code, it looks like ignored is being used to prevent watching files early on.

@benmccann
Copy link
Collaborator

This seems very related to #7363 as well. I think all the source files should be in a single directory we can watch and all the static files should be in a single directory. Just mixing everything together under root becomes difficult to handle in several scenarios where it turns into a bit of a mess

dhess added a commit to hackworthltd/primer-app that referenced this issue Oct 10, 2022
This fixes the issue described here:

#547 (comment)

It does so by configuring `direnv` to write its cache for this project
in `~/.cache/direnv/layouts`. This is a bit intrusive, but without
this workaround, `vite` commands search the local `.direnv` cache for
dependencies, and it adds about 10 seconds or more to every `vite`
command. Note that the fix comes from here:

https://github.com/direnv/direnv/wiki/Customizing-cache-location

Before falling back to this workaround, I also tried the following
Vite configuration tricks, but none of them worked:

https://vitejs.dev/config/server-options.html#server-watch
https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-exclude

There are multiple (somewhat) related issues currently filed on Vite
which suggest that convincing Vite not to look everywhere in the
project root directory is a known issue:

https://github.com/vitejs/vite/pull/8778/files
vitejs/vite#7363
vitejs/vite#8341
@patak-dev patak-dev added enhancement New feature or request performance Performance related enhancement and removed enhancement: pending triage labels Mar 10, 2023
@rtsao rtsao mentioned this issue Jun 21, 2023
4 tasks
@rkrisztian
Copy link

rkrisztian commented Nov 14, 2023

I agree, right now when I create a vanilla project from scratch, even files inside dist are watched. And you may use various tools that put temporary files inside other directories like build, maybe including cache files if you go for a more self-contained setup that does not litter into /tmp or into your home folder. When developers look at a generated vite.config.ts, they should be immediately aware that the file watches should be customized, by seeing a minimal configuration that at least excludes dist. Not after the fact that they recognize weird performance issues or reaching system limits on file watches.

In other words, for me the following helped, but minimally excluding dist is what I suggest in in a vanilla project, especially because everyone's setup may be different, e.g., this example does not give enough love to other IDEs like VSCode:

    server: {
        watch: {
            ignored: [
                `${__dirname}/dist/**`,
                `${__dirname}/build/**`,
                `${__dirname}/.idea/**`,
                // etc.
            ]
        }
    },

Edit: But of course, a white-list approach might be even better, if the filtering is simpler that way.

Right now the fact that we put files like index.html and public outside of src kind of complicates the whitelist approach more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Performance related enhancement
Projects
None yet
Development

No branches or pull requests

5 participants