vite icon indicating copy to clipboard operation
vite copied to clipboard

Support @parcel/watcher

Open rtsao opened this issue 2 years ago • 46 comments
trafficstars

Description

Chokidar is slow and known to have issues with file descriptor limits when working with large projects. Parcel's watcher is much faster out of the box and additionally supports watchman as a backend, which improves support for Windows and minimizes overhead for projects already using watchman.

Nuxt recently added support for @parcel/watcher: https://github.com/nuxt/nuxt/pull/20179

Suggested solution

Add option to use @parcel/watcher

Alternative

No response

Additional context

Related issues:

  • https://github.com/vitejs/vite/issues/11468
  • https://github.com/vitejs/vite/issues/2547#issuecomment-801528123
  • https://github.com/vitejs/vite/issues/12495
  • https://github.com/vitejs/vite/issues/8341
  • https://github.com/vitejs/vite/issues/11938

Twitter discussion https://twitter.com/patak_dev/status/1649060364431028226

Validations

rtsao avatar Jun 21 '23 16:06 rtsao

We've discussed this before (Discord thread) and are open to trying it in Vite 5. The main blocker for me is that @parcel/watcher isn't publishing precompiled binaries like lightningcss and esbuild, so it brings a larger published size by default.

bluwy avatar Jun 22 '23 06:06 bluwy

We've discussed this before (Discord thread) and are open to trying it in Vite 5. The main blocker for me is that @parcel/watcher isn't publishing precompiled binaries like lightningcss and esbuild, so it brings a larger published size by default.

Vite should not depend on a C++ compiler being present as that is a major pain point for users.

silverwind avatar Jun 22 '23 07:06 silverwind

How so? Vite is already using esbuild. And has ecosystem support for SWC and lightningcss.

bluwy avatar Jun 22 '23 08:06 bluwy

@parcel/watcher isn't publishing precompiled binaries like lightningcss and esbuild

@silverwind I think you misunderstood this comment. I guess you understood that comment as "@parcel/watcher isn't publishing precompiled binaries" and "@parcel/watcher uses node-gyp / C++ compiler". But I believe @bluwy meant to say "@parcel/watcher isn't publishing precompiled binaries for each OS/arch in a separate package like lightningcss and esbuild does".

sapphi-red avatar Jun 22 '23 15:06 sapphi-red

Ah, so it is one big package of precompiled binaries with all os and archs in the package. Not ideal for install performance, but at least requires no install-time compilation, so it may be acceptable.

silverwind avatar Jun 22 '23 15:06 silverwind

Just for my curiosity, any reason for not considering Turbowatch?

Mentioned in the original thread https://github.com/vitejs/vite/issues/12495#issuecomment-1602091976

It uses fs.watch under the hood by default and otherwise fallsback to other mechanisms, including Watchman.

gajus avatar Jun 22 '23 22:06 gajus

Just for my curiosity, any reason for not considering Turbowatch?

Mentioned in the original thread #12495 (comment)

It uses fs.watch under the hood by default and otherwise fallsback to other mechanisms, including Watchman.

From a maintenance standpoint, a pure JS module would be much better than C++ modules, which tend to require constant maintenance to not break because of ABI changes etc. It you check the issue tracker of parcel-bundler, there are a number of open issues about non-working platforms.

silverwind avatar Jun 22 '23 22:06 silverwind

That + it is very easy to add custom backends. Here is an example chokidar backend.

The default backend basically checks if fs.watch can be used, and if it can, then it uses it, or otherwise fallsback to chokidar.

The other thing worth mentioning is that Turbowatch implements file hashing on top of file change triggers provided by the backend. In practice, we've found that this dramatically reduces the number of unnecessary re-renders as compared to mtime based systems.

gajus avatar Jun 22 '23 22:06 gajus

Releasing separate npm packages per platform has been on my list for @parcel/watcher for a while, just haven't had time yet. If someone wants to contribute that, it would be helpful, otherwise I will get to it at some point soon hopefully.

which tend to require constant maintenance to not break because of ABI changes etc

This particular change is very rare actually. Node-API is ABI stable, so should not break between versions of node. There are a few very obscure platforms that we haven't implemented backends for yet, or we need to provide pre-builds for so you don't need a compiler to install (once we have separate packages per platform this will be much easier), but in general it is very stable. It's exceedingly rare that an issue comes up that is caused by the watcher itself. Implementing low level stuff like this in compiled languages with deep integration into the OS provides a lot of benefits, especially in regards to performance (e.g. watching and debouncing occurs on a background thread).

@parcel/watcher is extremely widely used at this point (3M+ downloads per week on npm alone), including by VSCode, Nuxt, Nx, Parcel, etc. If there are things Vite needs in order to adopt it, I'm happy to help where I can.

devongovett avatar Jun 23 '23 03:06 devongovett

Update: I've released @parcel/watcher v2.2.0 which publishes individual packages per platform/architecture, and includes prebuilt binaries for a bunch more as well. See the release notes for a full list.

devongovett avatar Jul 02 '23 03:07 devongovett

I made two branches to test it out:

  • @parcel/watcher: https://github.com/vitejs/vite/tree/bluwy-parcel-watcher
  • fs.watch: https://github.com/vitejs/vite/tree/bluwy-fs-watch

Made fs.watch mainly because we need a wrapper for @parcel/watcher to work, and that wrapper can be easily applied to fs.watch too. Both branches are not optimized and failing in tests now though (pnpm test-serve).

For @parcel/watcher, I'm getting FATAL ERROR: HandleScope::HandleScope Entering the V8 API without proper locking in place. For fs.watch, the tests are hanging somewhere. Both are likely because they are creating many watcher instances to fit Vite needs, where we need an API to watch out-of-project-root paths.

bluwy avatar Jul 02 '23 08:07 bluwy

The Entering the V8 API without proper locking in place error appears to be due to the tests running inside worker threads. If you set the vitest threads configuration to false it does not occur. This hasn't been something we've needed yet, but I can look into it. Sorry about that!

devongovett avatar Jul 02 '23 15:07 devongovett

I did some work to support multiple threads over in https://github.com/parcel-bundler/watcher/pull/146. There is an alpha version published if you want to try it out: v2.2.1-alpha.0. I ran the vite tests from your branch with this version and it seemed to work better. The remaining failures seem related an API change for the on method in your Watcher class that aren't accounted for in @vitejs/plugin-vue for example (causing "eventHandler is not a function" errors later). Let me know if this works better for you. I'll be doing a bit more testing on my side as well to ensure this is stable before releasing.

devongovett avatar Jul 05 '23 16:07 devongovett

Thanks! I forgot to give an update that the threads: false tip helped. I've fixed the plugin-vue issue and pushed it to the branch. I'm not getting the V8 errors now, but there are existing HMR test fails which I haven't figure out yet.

I'm also curious on your thoughts if the Watcher class and how it calls subscribe is performant. I do wish @parcel/watcher could provide that Watcher interface directly instead (similar to chokidar), but maybe that's out of scope of the project.

bluwy avatar Jul 05 '23 16:07 bluwy

Yeah it's probably not the most performant. On macOS it's probably fine, but on Linux there is a pretty low inotify watcher limit and watchers are not recursive by default so we have to traverse the directory hierarchy and watch each sub-directory manually. I noticed that you're calling subscribe on a lot of overlapping directories inside the project, rather than watching once from the root of the project. This will currently result in common directories being traversed and watched more than once.

If possible I'd recommend watching from the root of the project instead of watching each directory. You could also potentially use a similar interface to what you have but keep track of all the directories that were watched and find the minimal set of common root(s) between them to actually watch. We could potentially do something like that in @parcel/watcher itself eventually, but I might even implement that as a JS wrapper anyway - not sure if there is much benefit to doing it in C++.

devongovett avatar Jul 05 '23 16:07 devongovett

It currently also adds the project root here

https://github.com/vitejs/vite/blob/f385f2025c63f8804795ee359e53e4e76fcc27d1/packages/vite/src/node/server/index.ts#L356

So the subscribe shouldn't be called too much as there's an overlap guard, unless it's out of root. In Rollup/Vite, we have a this.addWatchFile API for plugins to watch and depend on certain files, so we need a Watcher.add function to dynamically handle those. A potentially not-so performant part with this is that we can only watch directories, so addWatchFile can only watch the file's directory and indirectly watch more than it's needed.

@ArnaudBarre also had an idea that we only watch what's needed. For example, at the start we don't watch anything, but as we start to serve the app and crawl the transformed files, we add each of them to be watched. With the current Watcher class, this might not be great for perf.

bluwy avatar Jul 05 '23 17:07 bluwy

Apologies for the delay! I have now released @parcel/watcher v2.3.0, which includes support for worker threads, a new kqueue backend to support freebsd, and a wasm backend to support stackblitz. https://github.com/parcel-bundler/watcher/releases/tag/v2.3.0

devongovett avatar Aug 24 '23 04:08 devongovett

I've took another stab to use @parcel/watcher (https://github.com/vitejs/vite/pull/14731). We need to keep compatibility with chokidar's API for now to not break the ecosystem, which I implemented in the PR, but it's becoming a larger undertaking than expected.

The wrapper to use @parcel/watcher is fairly big that it might be worth creating a separate library too, so for now we'll be moving this out of the Vite 5 milestone. We can definitely experiment this more in future minors (e.g. optional peer deps to enable it) and then create a migration path for the ecosystem to move to this new watcher API.

bluwy avatar Oct 23 '23 08:10 bluwy

@bluwy If compatibility with chokidar is desirable, what was the reason for not considering Turbowatch?

We've been using it now for ~7 months without an issue.

gajus avatar Oct 23 '23 13:10 gajus

Personally the package size is too big (though Vite bundle dependencies and I have not measured what the size would be). But it also depends on chokidar, and in that case it would be simpler if we use chokidar directly? It looks certainly robust but we're not going to use all of its features, and we usually try to balance the cost of functionality per package size in Vite.

bluwy avatar Oct 23 '23 13:10 bluwy

Interesting. Was not even aware of the package size.

If zx gets updated (or if it is made a peer dependency), the size drops to 10MB.

Removing of the other dependencies though would be challenging without compromising the developer experience.

Could maybe separate the core from the binary. Happy to explore if there is interest.

gajus avatar Oct 23 '23 13:10 gajus

Please let me know what would help. We can probably put at least some of what you need in @parcel/watcher itself. I've already put in quite a bit of work to support Vite, so it would be awesome to see it through! 🙂

devongovett avatar Oct 24 '23 07:10 devongovett

@devongovett Appreciate the work as always! I think what we need is an easy migration path to use it, since it's usually an implementation detail so we don't want it to be too breaking. A couple ideas:

  1. Create a chokidar fork with the same API but uses @parcel/watcher.
  2. Or create a new (and better) watcher interface, and Vite can easily create a chokidar compat layer.

The packages could be done by anyone though. We can then support as an optional peer dep and push it. However I'm a bit tied on bandwidth at the moment 😬


What would be nice in regards to @parcel/watcher is:

  1. https://github.com/parcel-bundler/watcher/issues/92
  2. A way to identify if create or delete is caused by a file or directory (chokidar has this, though I don't have a large usecase for it)

Additional notes:

  1. We initially thought that @parcel/watcher was similar to chokidar, but it seems to be closer to fs.watch so it's not the anticipated amount of work we expected for the milestone. Though I tried my best attempting it.
  2. An ideal case would be to have Rollup adopt @parcel/watcher too, and I think the requirements would be the same as us.

bluwy avatar Oct 24 '23 10:10 bluwy

From what I know of Vite HMR, I don't think 1. and 2. are needed. Plus I think that this change would be the occasion to remove the default watch of the whole working dir that then need some edge cases. This is something that I like to work on, but didn't get to it during v5 beta

ArnaudBarre avatar Oct 24 '23 18:10 ArnaudBarre

1 and 2 is needed because we expose server.watcher. I've also thought about the "watch lazily" plan but ultimately I dont think it's feasible because:

  • Feels like a big overhead on startup as you're watching many tiny files/directories in a burst, and need tooling to consolidate and deduplicate watchers. (This is also where a chokidar like interface makes it easier to manage)
  • We cannot detect added/removed tsconfig.json within the root that would've affected .ts files. (discussed with dominik about this one)

bluwy avatar Oct 25 '23 04:10 bluwy

Chokidar also supports glob pattern for file watch and ignored option as well but I don't see any glob support by Parcel Watcher (at least not from the docs on the readme).

To be fair though, it seems that the Chokidar author wanted to remove globs in this open Chokidar version 4 PR but I doubt it would be merge anytime soon since the PR is 1.5 year old at this point... which reminds me globs feature was removed from rimraf not long ago but quickly added back because too many users begged for the glob support :). So anyway, does Parcel Watcher support globs and if not then how would Vite go about it?

ghiscoding avatar Oct 25 '23 14:10 ghiscoding

Yes, glob ignores are supported. They're pretty efficient too - they get compiled to a regex and matched in the C++ layer (off main thread). https://github.com/parcel-bundler/watcher#options

devongovett avatar Oct 25 '23 15:10 devongovett

Yeah it's probably not the most performant. On macOS it's probably fine, but on Linux there is a pretty low inotify watcher limit and watchers are not recursive by default so we have to traverse the directory hierarchy and watch each sub-directory manually.

Node just made native recursive watching available on Linux. It was originally added in Node 20, but had some bugs, so you couldn't use it just yet. It seems to be working now as of Node 20.13/22.1+. Perhaps @parcel/watchers could adopt it now?

benmccann avatar May 20 '24 23:05 benmccann

We have already used the same OS APIs that node is using for years. Their implementation would be subject to the same OS limits.

devongovett avatar May 20 '24 23:05 devongovett

Ah, yeah. I guess you would need to implement fanotify to address this (https://github.com/parcel-bundler/watcher/issues/87)

benmccann avatar May 21 '24 00:05 benmccann