search

paulmillr / chokidar

Minimal and efficient cross-platform file watching library
https://paulmillr.com
MIT License
10.8k stars 574 forks source link

v4 fixes #1304

Closed 43081j closed 3 months ago

43081j commented 7 months ago

This is just a a draft PR for me to track any changes I do to the v4 branch.

So far:

cc @paulmillr

43081j commented 7 months ago

fixed two more:

if you're curious, i've been detailing each of the fixes in the commit messages

43081j commented 7 months ago

we're down to 3 failing tests now.

this change:

paulmillr commented 6 months ago

FYI there's recursive: true option that has been present around since v10 or so, and it seems to allow efficient watching. This should be investigated.

As for CI, better to test v18, v20, and that's it - no need for v14, v16.

43081j commented 6 months ago

one thing i just updated:

in linux, the inode of a watched file can change when the file is replaced. it seems that same behaviour happens in macOS too, but we only account for it in linux.

i changed it to also re-watch in macOS and it fixed the safe-edit test (on macOS)

paulmillr commented 6 months ago

The tests are shit. Maybe we move tests to micro-should? and make package ESM-only for simplicity?

43081j commented 6 months ago

tbh my preference would actually be one of these:

and either way, i'd probably rework the tests themselves to make the cases clearer (not sure how exactly off the top of my head, but it has been difficult at times to know what exactly a test is testing).

ESM only: i would be all for this, e.g. i did this with chai not long ago and we haven't seen a huge amount of problems from consumers (lot of discussions came up, issues, etc. but they were all docs related). on the side, i am working on a cjs-bundle npm org for holding commonjs bundles of esm-only packages, so we may be able to use that here too for people who want to upgrade but are stuck with commonjs.

also an update - i have one failing test remaining locally, but it also fails in master. i'd still like to fix it but its proving difficult to debug, will let you know if i get any further with it though

43081j commented 5 months ago

FYI it finally passes tests in ubuntu with node 20.x

all other targets fail right now

43081j commented 5 months ago

@paulmillr maybe im missing something or maybe i've had a mini-revelation: can we just not use fsevents anymore?

the node docs:

On macOS, this uses kqueue(2) for files and FSEvents for directories.

so maybe having the fsevents package is an unnecessary layer we can just delegate to node itself?

we could remove the fsevents-handler if thats the case...

i also realised the reason master passed tests on my macbook was because it was failing to install fsevents silently and was using the nodefs handler instead. if i force the v4 branch to do the same, all tests pass!

paulmillr commented 5 months ago

@43081j we can definitely try dropping fsevents if it’s fast enough without it. Need proper benchmarks tho. like 500k files or something.

43081j commented 5 months ago

👍 makes sense to me, i'll see where i get to once i have ci passing at least

43081j commented 5 months ago

@paulmillr this should pass CI now at least

i had to rework the "thirtythree files" test to wait for each file individually to call the spy..

does that still test what you were originally trying to test? i'm not sure we can easily have a non-flaky test that just writes them all in one go

on windows, it fails most of the time. on ubuntu, it fails sometimes under node 20 but never on node 21.

its just a flaky test in general i think, i've seen the same failures intermittently in master too. this was the only way i could make it stable

i'll look into perf when i get chance next

paulmillr commented 5 months ago

Great work! I didn't think you will actually fix it. Could you add node v21 to the CI as well?

does that still test what you were originally trying to test

hard to say. I guess we'll just use your change and spectate.

43081j commented 5 months ago

sure, have updated CI. will see if 21 passes now 👀

thats what i've been using locally mostly so it should be fine

as for dropping fsevents btw - i've been testing this on two different mac machines (an up to date macbook pro, and a mac VM in the cloud). both seem happy using node's built in watcher

still need to perf test it though, will take a look at that at some point

43081j commented 3 months ago

I'll pick this back up soon 👍 I haven't forgot

Just got preoccupied with a bunch of other repos but id like to get this done still

Just fyi

43081j commented 3 months ago

@paulmillr any chance we could setup way to publish next tags in npm to try this out a bit?

similar to what i have in chai-as-promised: https://github.com/chaijs/chai-as-promised/blob/master/.github/workflows/npm-publish.yml

means we could make releases in github to trigger prerelease publishes to npm. i could start to try this out in some larger frameworks/libs that way instead of having to link them all locally

meanwhile im still trying to fix the windows failures. it'll be timing in tests again, fairly sure the functionality itself is fine

edit: i think we get passing builds now too. hopefully they are consistent 🙏

paulmillr commented 3 months ago

any chance we could setup way to publish next tags in npm to try this out a bit?

can’t we just point to github in package.json? it allows specifying branches and commits. but we would need to have js output in the repo for now

43081j commented 3 months ago

aha fair point, i totally forgot we could do that

i see we had one macos failure too, its the same test thats been a bit flaky. will take a look at that

benmccann commented 3 months ago

FYI there's recursive: true option that has been present around since v10 or so, and it seems to allow efficient watching. This should be investigated

Yes. Note that the Linux implementation was added in Node 20 and had bugs when initially added. I would recommend using this API on Linux only on Node 20.13/22.1+. See https://github.com/nodejs/node/pull/51406 and https://github.com/nodejs/node/pull/52349 for more details

43081j commented 3 months ago

i haven't added the recursive flag yet and think it makes more sense to do that later, once we have a working build to at least start trying out in consuming projects

the builds seem somewhat stable now. so ill start testing it out in some places later this week

have done the other suggestions

paulmillr commented 3 months ago

What do we need to do to release v4? Do you have a plan in mind?

paulmillr commented 3 months ago

The test seems to be failing.

43081j commented 3 months ago

It'll be the unlink dir test, it's a bit flaky. I'll have a look into it again soon 👍

My suggested plan is this:

The big job really is the first step, just proving it out within popular tools.

paulmillr commented 3 months ago

I don't know who we should talk to.

I think we should just pull the plug and release v4. Would it be stable? Not really. Would people try it? Probably.

43081j commented 3 months ago

i can handle some of that i think. storybook, vite and astro would be good ones to try out and i know enough people at each that i can get some feedback

i think it makes sense to publish it on a next tag in npm once i get the flaky test passing (publish --tag next)

then give me and others some time to trial it out, and aim to release a stable version in the next month

benmccann commented 3 months ago

If we're pretty sure that the flaky test is because of the test and not the library then I wouldn't say we need to hold up a pre release for deflaking the test. We could cut the pre release and let people start testing it out now while work on the flakiness happens in parallel.

Should we go ahead and merge this PR into the main v4 PR/branch now?

43081j commented 3 months ago

i'd agree with that

i don't think we should be publishing a stable version but we should publish a next tag for sure so people can start trying it out

i'll ask around and start testing it myself against some tools/frameworks too

i'll also fix the test eventually (haven't managed to reproduce the failure locally on any of my laptops so far)

benmccann commented 3 months ago

+1 to publishing a next version before a stable version

benmccann commented 3 months ago

storybook, vite and astro would be good ones to try out and i know enough people at each that i can get some feedback

Just FYI, there was some discussion in the Vite repo about whether to stick with Chokidar or not (https://github.com/vitejs/vite/issues/13593). I don't think any decision has been made one way or another though and the PR to switch away from it was closed as stale (https://github.com/vitejs/vite/pull/14731).

One thing that was interesting that came out of that discussion was that @parcel/watcher does its watching on a separate thread natively. I wonder if that's important for performance if chokidar could get some of the same benefits from using a worker. Might be something interesting worthy of a benchmark

43081j commented 3 months ago

true, we should look into that once this lands 👍

on a side note - one of the big consumers is mocha. as of 10.x it still uses commonjs in the CLI. does make me wonder if we should be publishing cjs/esm to help drive adoption, or if we stick with what we have and try help mocha move towards ESM (but will be a long job).

benmccann commented 3 months ago

Having the source written in JS rather than TS was quite nice in terms of package size because it meant that source maps weren't required. I'm afraid the package is going to get much larger with the move to TS. If an ESM-only version is published there will definitely be complaints and folks who don't upgrade. And it certainly wouldn't be hard to dual publish, but that would double the package size again. If you're just running on your own machine that doesn't necessarily matter, but there are things like https://learn.svelte.dev/ that use chokidar in the browser where download sizes do matter.

Note that if you write the code in JS, you can still keep type checking the code with TypeScript. That's what we do for Svelte/SvelteKit and it's worked well for us. So it's probably a controversial/unpopular view and my vote doesn't really matter here, but I'd vote to write the code in JS instead of TS to avoid all the build tooling and additional package size from source maps :laughing:

paulmillr commented 3 months ago

does its watching on a separate thread natively

node.js io is async, which means it also uses separate threads...

paulmillr commented 3 months ago

@benmccann we can just not generate the source maps. That would totally keep the size reasonable.

As for increased package size with hybrid/maps, the new amount of sub-deps would more than compensate for this.

benmccann commented 3 months ago

Exciting to see this merged!

node.js io is async, which means it also uses separate threads...

The io itself uses separate threads, but there was some mention that crawling the directory tree to add inotify watchers on each of the subdirectories on startup is time consuming and some portion of that happens in the main JS thread.

@benmccann we can just not generate the source maps. That would totally keep the size reasonable.

It would make bug reports pretty annoying. Anything with a stacktrace would have the line/col numbers wrong.

As for increased package size with hybrid/maps, the new amount of sub-deps would more than compensate for this.

Woo! Yeah, it's great to see it slimmed down!