[DRAFT] v4 fixes

[43081j]

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

So far:

• Fixed an async bug in the tests so they now run properly (but fail elsewhere)
• Upgraded rimraf to fix a race condition (old rimraf was not awaitable, so was actually completing after the test setup)

cc @paulmillr

[43081j]

fixed two more:

• dynamic import of chokidar in tests needs to be the exact path, rather than a directory
• race condition in the close test

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

[43081j]

we're down to 3 failing tests now.

this change:

• implement ignoredPaths again using relative path computation to figure out if it is a child or not
• rework anymatch to be much stronger typed and less generic (since we didn't use the extra functionality it had)
• fix the lint script and add TSESLint recommended config

[paulmillr]

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]

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]

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

[43081j]

tbh my preference would actually be one of these:

• upgrade mocha/chai
  • latest chai is esm only
  • latest mocha supports esm out of the box
• use node's built in test runner
  • will require a minimum node version in CI (not sure which)

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]

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

all other targets fail right now

• Windows (WSL), fails miserably. not too sure whats going on there
• MacOS, seems to fail on symlink tests. i spent a long time debugging them but didn't really get anywhere
• Linux, seems to fail on node 18 but i don't have a linux machine to reproduce this (will try spin a vm up some time maybe)

[43081j]

@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]

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

[43081j]

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

[43081j]

@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]

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]

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]

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]

@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]

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]

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]

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]

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]

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

[paulmillr]

The test seems to be failing.

[43081j]

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:

• try it out manually with some larger consumers (frameworks, popular libraries, etc)
• publish a prerelease at some point to npm
• ask around for community feedback, people to try it out, etc
• eventually publish stable

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

[paulmillr]

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]

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]

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]

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]

+1 to publishing a next version before a stable version

[benmccann]

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]

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]

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: