Version 4: Typescript, ESM, glob removal, drop node <18 #1195
Conversation
paulmillr
commented
Jan 18, 2022
•
paulmillr
commented
- Release readdirp v4
- Changelog
- Release chokidar v4
paulmillr
changed the title
paulmillr
changed the title
|
@paulmillr is this alpha release blocked ? Thanks a lot for your great work. |
|
Nah -- someone just needs to find some time to debug it. |
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`).
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.
paulmillr
force-pushed
the
master
branch
2 times, most recently
from
368cb02
to
7c50e25
Compare
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.
We were running ESLint against `*.js` (the default) which no longer exists, so `--ext` has been added to make it check `*.ts` files. Alongside that, TSESLint has been added and the recommended config enabled. For now, these two rules are disabled: - `no-explicit-any` - until we strongly type everything - `no-unused-vars` - probably permanently, using typescript's own `noUnusedLocals` instead
This basically does two things:
1. Adds `WatchHelper` to some of the methods for a stronger type
2. Reworks how we add/remove ignore paths ("matchers")
In the latter, thanks to a previous change where we can have "matcher
objects", we need to iterate through existing ignore paths to make sure
we don't duplicate them.
Similarly, when removing a "path", we need to find any matcher objects
which use the same path and remove those too.
Basically introducing `addIgnorePath` and `removeIgnorePath` instead of
mutating the set directly.
As per the nodeJS docs, the built-in `fs.watch` will now use `fsevents` under the hood for watching directories. This means we should no longer need the `fsevents` dependency and can instead just delegate to node. Some stability changes have been made to the tests, too. Generally waiting for explicit conditions rather than arbitrary delays.
On windows, a symlink can be created as a `file`, a `dir`, or a `junction`. Meanwhile, on unix-like systems, this type doesn't seem to matter much (or is ignored). By default, the type will be `file` on windows. Due to this, all directories we symlink in tests are actually created as "files" which leads to all sorts of permissions errors. This change basically makes it explicit that the symlinked directories are in fact directories (`dir`), thus making tests pass.
This test is incredibly flaky across different platforms, node versions, etc when we try write all the files at once. Instead, we now write a file at a time and wait for the spy to have been called before moving on. Not quite as accurate as real world but seems to be the only way to make this stable.
This adds node 21.x to the matrix in our CI workflow (making it now run on 18, 20 and 21).
Attempts to fix some timing issues in tests by waiting for watchers to be ready before continuing with the test cases.
Bumps the version to `>= 18` and replaces `21` with `22` in the CI workflows.
[DRAFT] v4 fixes
|
@43081j there are a few issues:
|
paulmillr
changed the title
|
@benmccann how flexible are you in terms of ESM-onliness? |
we should be able to use node's test runner now (since 18.x at least, maybe even 16.x)
these are probably both solved by us updating and cleaning up some dependencies. ill take a look esm vs cjsif we did want to support commonjs, i'd push for using tshy i use that in many other oss repos i help maintain and it makes the whole dual package business pretty straight forward would be nice to see the package size difference compared to v3 too |
|
I have made many hybrid packages manually and it's nothing hard. It's just launching tsc two times with different configs. See https://github.com/paulmillr/jsbt
|
|
as a dev dependency keep in mind. the important thing tshy does is create the package i don't mind either way as long as things like arethetypeswrong say our package is setup correctly (if it doesn't, its likely ts consumers will get build errors in some configurations) |
|
|
typescript, node and many bundlers will now assert that the imported path is in the export map so we should set it up but im fine with us doing it by hand too, whatever we can all agree on 👍 i wouldn't export extensionless paths etc, i'd pretty much export the on-disk files (aliasing for cjs if we dual-package, and exporting them directly). i think we're in agreement 👀 |
Switches to using the built-in node test runner and drops mocha. Temporarily also introduces a `TEST_TIMEOUT` as `--test-timeout` is only available in 20.x and above, but we run CI in 18.x.
Adds a small delay in some tests after the initial path has been added (to avoid the FS de-duping/cancelling events).
test: switch to node test runner
Upgrades eslint and switches to using the new flat config structure.
chore: upgrade to flat eslint configs
|
We also need to make this ESM and upgrade https://github.com/paulmillr/readdirp/ |
i have a branch where i've tried to remove though it has a few test failures so i was planning on trying to fix those what do you think? unrelated - the same failure as my last few PRs happened again here. seems to be windows specific 🤔 i'd like to solve that one somehow |
|
ideally we'll just use node built-ins, ofc. Try readdirp-ing 400K-file directory and seeing how's the speed with both solutions. I imagine nodejs is still shit. Keep in mind that readdirp uses streams etc. |
|
thats true, i'll try it out on a larger project if i can find one some place 👍 |
|
i did some benchmarking, and it seems like readdirp recursively visits some symlinked directories, whereas node won't in a test repo:
it seems the missing items are already part of the in a directory that doesn't hit that problem, node seems faster: |
|
Symlinks are definitely important. |
im not saying node is ignoring them. readdirp is duplicating them from what i can tell
the extra 89,145 entries seem to be files in symlinks certainly are traversed via node |
