v4 fixes #1304
v4 fixes #1304
Conversation
* add `workflow_dispatch` * specify `FORCE_COLOR: 2` for colored output * update `actions/checkout` to v2 * update `actions/setup-node` to v2 * add Node.js 16 * reduce CI matrix
Update CI config
Remove unused devDependencies
Added some more info to `Why was chokidar named this way?`
Update README.md
Avoid nested async/await and Promise constructors that caused parts of the old tests to continue running and interfere with subsequent tests
The `fs.mkdir` function does not take an `options` object until Node 10
Fix some tests
fix readyCount logic
…bDirs handle MustScanSubDirs
…es-1299 update fs.FSWatcher types to satisfy nodejs versions >= 16; fixes paulmillr#1299
The dynamic import causes the `describe` blocks to happen later, which means the `after` and `afterEach` have already run (and cleaned up) at that point. It also turns out the old version of rimraf wasn't awaitable, so it has been upgraded in this change.
This reverts to what the test is in `master` but fixes an async race condition. Essentially: ```ts await waitForWatcher(watcher1); // at this point, the ready event has fired for watcher1 _and_ watcher2 await waitForWatcher(watcher2); // this never gets reached because it fired before we added the event // handler ``` Awaiting both in a `Promise.all` fixes this since the event handlers will be attached immediately.
Since we're locally importing this, node will not resolve the import path as if it is an NPM package (i.e. it will not infer the `main` path, or package exports). Instead, we need to import the exact path it seems (`lib/index.js`).
|
fixed two more:
if you're curious, i've been detailing each of the fixes in the commit messages |
This reverts the test for ignoring events after close, since it seems the original passes fine and makes this consistent with the other tests again in structure.
This reworks the anymatch function to be a little less generic since we now know exactly what we want to call it with. This means we have much clearer/stronger types. On top of that, support for ignored paths has been implemented again (it was removed with a `TODO` until now, in v4). Keep in mind the way we store ignored paths may change in future so we can more efficiently access and remove them without the need for full iterations through the set.
|
@paulmillr any chance we could setup way to publish similar to what i have in chai-as-promised: 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 🙏 |
Attempts to fix some timing issues in tests by waiting for watchers to be ready before continuing with the test cases.
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 |
|
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 |
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 nodejs/node#51406 and nodejs/node#52349 for more details |
Bumps the version to `>= 18` and replaces `21` with `22` in the CI workflows.
|
i haven't added the the builds seem somewhat stable now. so ill start testing it out in some places later this week have done the other suggestions |
|
What do we need to do to release v4? Do you have a plan in mind? |
|
The test seems to be failing. |
|
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. |
|
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. |
|
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 then give me and others some time to trial it out, and aim to release a stable version in the next month |
|
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? |
|
i'd agree with that i don't think we should be publishing a stable version but we should publish a 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) |
|
+1 to publishing a |
Just FYI, there was some discussion in the Vite repo about whether to stick with Chokidar or not (vitejs/vite#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 (vitejs/vite#14731). One thing that was interesting that came out of that discussion was that |
|
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). |
|
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 😆 |
node.js io is async, which means it also uses separate threads... |
|
@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. |
|
Exciting to see this merged!
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.
It would make bug reports pretty annoying. Anything with a stacktrace would have the line/col numbers wrong.
Woo! Yeah, it's great to see it slimmed down! |
This is just a a draft PR for me to track any changes I do to the v4 branch.
So far:
cc @paulmillr