fix: watch mode using chokidar v4 (#5355)

[Arnesfield]

Glob paths are no longer supported by chokidar starting at v4.

This update works around this by resolving watchFiles and watchIgnore to valid paths and creating a list of matchers to determine if the files should be allowed or ignored through the chokidar ignored match function.

This commit reverts changes from 8af0f1a so that the watched file changes are not fixed to the current directory.

Additional note: when a watchFile path is removed while chokidar is watching it, recreating the watchFile path does not trigger events from chokidar to rerun the tests.

PR Checklist

• Addresses an existing open issue: fixes 🐛 Bug: watching files outside of CWD is broken #5355, fixes 🐛 Bug: Not able to watch files with huge node_modules anymore after bumping chokidar to v4 #5374 (@mark-wiemer added 5374)
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: JoshuaKGoldberg / name: Josh Goldberg ✨ (7d716db)
• ✅ login: Arnesfield / name: Jefferson Rylee (4f3150d, 539d4de, 0b1babc, bad7b36, 5e39fae, 24988ef, 4860486, fcc839f, 3660ed2)
• ✅ login: mark-wiemer / name: Mark Wiemer (eb2792c, 5514fe2, 8af5d0b, 2a9c88c, ca7d630, c3dd839, caca650, 84b27a9, f897e65, 9e2a41e, 00bb46a, fea5476)

[Arnesfield]

Might need to rethink this a bit. Currently, running into some issues:

• The test reruns test when file matching --watch-files is added doesn't pass
• The watchFiles for this test are: **/*.xyz
• From what I understand, the actual test result looks like it has an additional rerun (3 instead of the expected 2) which I think is probably due to the directory being created (lib/ then lib/file.xyz)
• But ignoring the directory in chokidar means its contents are also ignored
• Need a way to not ignore a directory (lib) but also only allow contents to trigger a rerun (*.xyz)

[JoshuaKGoldberg]

The changes seem reasonable to me at first, but they're not covered by tests at all. I think we'd want to have at least one integration test covering the behavior.

Maybe:

1. Programmatically creates a big enough test case project to crash chokidar v4's watch mode
2. Starts Mocha in watch mode
3. Waits until it logs the expected message (and asserts it doesn't log an EMFILE or anything like that)
4. Closes Mocha

WDYT?

[Arnesfield]

Hi @JoshuaKGoldberg, thanks for the response.

Actually, the changes are really only meant for handling glob paths for watchFiles and watchIgnore and not so much about the EMFILE issue or crashing chokidar. I'm thinking that those issues should have their own pull requests and set of changes instead of here.

To recap, since chokidar v4 dropped support for globs, this caused issues where files are not correctly being watched. Before 8af0f1a, the watchFiles were being passed to chokidar directly, essentially entrusting the glob path handling to chokidar. After 8af0f1a, which upgrades chokidar to v4, these occurred:

• Glob paths for watchFiles and watchIgnore no longer work as expected.
• The watcher instance is now only watching file changes on the current directory (chokidar.watch('.')) instead of the patterns from watchFiles.
• The watcher instance no longer ignores patterns from watchIgnore.
• Watch mode become noticeably slower to start, probably due to the glob.sync() calls, especially for the watchIgnore patterns which would normally include node_modules.

These are the issues that these changes aim to address which are mostly fixes for the watched files, and I think that the EMFILE issue or testing the limits of chokidar would be out of scope for this (even though they're still more or less related to the watch mode).

While the watchIgnore patterns being ignored with this update could probably help with the EMFILE issue, the issues mentioned could still occur if the user's machine has a lower resource limit or if it does have too many files being read (e.g., by another program), and those could be caused by something outside of mocha/chokidar.

Please let me know what you think. Thanks!

[JoshuaKGoldberg]

The assorted recent watch mode issues for reference:

• 🚀 Feature [watch mode]: begin running tests immediately instead of waiting for watcher ready event #5372
• 🐛 Bug: Not able to watch files with huge node_modules anymore after bumping chokidar to v4 #5374
• 🐛 Bug: watching files outside of CWD is broken #5355

If it's easiest to fix >=1 of these all together in one PR (as in, the changed areas of code intersect a lot). I'd say go for it. 🙂

[Arnesfield]

The list of issues seem about right to me, where the first 2 are more or less about the watch mode taking some time to start than before and the last being the main issue. I think the changes here now should already address these issues, unless I missed something. Let me know should we need to update a few things or if there's anything to do on my end, thanks!

[JoshuaKGoldberg]

Great! Can you add test(s) to verify the changed behavior? #5379 (review)

[Arnesfield]

Added the following tests:

• Test for watched files from outside the current working directory
• Test reruns with a thousand watched files

Though, I'm not too confident with the approach for the last added test. Let me know if we should change how we go about it, thanks.

Edit: Well, the last test seems to have failed since it kind of relied on the machine's performance. I'll work on a fix for it and limit the change to a single file.

[mark-wiemer]

Next step here will be ensuring the test failures are nothing new (ref #5361), then I can review the code and if I approve I'll ping Josh to do the same :D

[Arnesfield]

Looks like the test for 1000 watched files is failing due to a timeout error (tests have a timeout of 10000). I tried reducing it to 100 files and while it seems to pass, it does not really test the limits of possibly huge projects with thousands of watched files, so it has been removed instead.

Though, the test for watched files outside the current working directory is kept as it is one of the main behaviors that we probably would want to keep in check.

[mark-wiemer]

Going to stress test this one as we saw failures in 5 out of 30 runs on main branch with our --watch tests, ref #5361.

Ran 20 test runs on my branch

5/20 runs failed:

• One known issue with --watch (run 4048)
  • This fails even more on main branch so this PR is an improvement if not a complete fix, still investigating
• One newly-discovered error with --parallel (run 4051)
  • Given we've only done 30 runs before this batch of 20 runs, I'm not too concerned about this, but will check it out
• Two fails due to a known unrelated issue (run 4039 and run 4049)
• One downstream error (Coveralls, run 4046)

Next steps:

• Learn more about Chokidar code and Mocha test architecture
• Try to reproduce these issues and fixes locally
• Review this PR in more depth (I read it but will need to read it a few more times given the significance of the changes)

[Arnesfield]

Pushed up an update to handle file paths under the provided watchFiles and additional tests for matching glob paths.

• The implementation has been refactored now and PathPattern has been updated to have Sets for the paths and globs (instead of an isGlob boolean).
• Allowed and ignored file path results are now cached to a map (with a size limit of 1000) to skip re-matching already matched file paths.
• Also added is-path-inside as a dependency to check for parent paths in the chokidar ignored match function.

Let me know should there be any changes needed or if I missed a thing or two, thanks.

Edit: I'm not sure why the netlify deploy preview is failing and how to resolve it. Clicking on it takes me to the netlify page with this permission error:

7:51:56 PM: You do not have permissions to view logs for this resource

[mark-wiemer]

Thanks! You can ignore Netlify, that's the host of mochajs.org, so we only use it for changes to the website, and I think only contributions by trusted contributors will pass the deploy preview. It's not a blocker to approval or merging in any sense :)

[mark-wiemer]

I'll review this tomorrow :)

[mark-wiemer]

Hey @Arnesfield, I've been digging into this one and I can't get over how complex it all feels. Even our current (broken) glob parsing in this file is dozens of lines, it seems we should be using the glob library more directly in some way to handle all of this for us. The Chokidar v4 upgrade docs have this:

// other way
import { glob } from 'node:fs/promises';
const watcher = watch(await Array.fromAsync(glob('**/*.js')));

Is something like that not possible for us? If not, why? I'll continue researching, right now I'm looking at how Chokidar v3 handled globs for a reference point.

Ultimately, if we do need this complex code, I'm going to ask for a lot of unit tests. Because Chokidar used to handle glob parsing for us, we never stress tested it, at least that's what I'm seeing from our current coverage.

[mark-wiemer]

Looks like Chokidar v3.5.3 accomplished its glob support with anymatch, which hasn't been updated in 3 years but still gets 58M installs a week and has 99% code coverage. I'm not aware of any security issues with it from searching the GitHub Advisory Database, and it only has two dependencies. @JoshuaKGoldberg what are your thoughts on using anymatch instead of custom code here? I'm in favor of it given the complexity and low test coverage we currently have.

[Arnesfield]

Hey @mark-wiemer, thanks for the response.

Yeah, using fs.promises.glob('**/*.js') would retrieve the file paths that match the **/*.js pattern and chokidar would watch those paths. Unfortunately, it wouldn't be watching any directories, and so if a new file is added like src/file.js, the watcher wouldn't emit events for it, and thus wouldn't rerun the tests. Using '**/*' would match directory paths, but for some reason, it won't emit events for file changes that are in the current directory. Additionally, doing fs.promises.glob() wouldn't be ignoring paths from watchIgnore, so it would even load files in node_modules, which can get pretty slow. Only filtering them afterwards just wouldn't be great. There's all that while also needing to be compatible with existing configs and I don't think we'd want projects updating their watchFiles and watchIgnore or anything like that.

What I think makes the chokidar v3 API very much different from the v4 API is that it seems they kind of prefer watching directories (or exact files) and only ignore the other paths via an ignored function (e.g., ignore paths that do not have a .js extension). At least, that's the impression I got.

Personally, I'm alright with any solution that would be better than this as long as the issue gets resolved. There's also the option to downgrade back to chokidar v3 until a major release like mocha 12, but I'll leave the decision up to the team here.

[mark-wiemer]

Hey @Arnesfield, pushed a bit of cleanup and some more docs. Globs are new to me and this is my first big PR on this repo, but I'm happy to approve now :) pinging @mochajs/committers for a second opinion, and of course we'll give you time to review the changes I made to this branch. Thanks for the help, this is a great bugfix and actually a boon to performance! (See #5374 for perf notes)

[mark-wiemer]

Test failures are known issues unrelated to this PR, ref #5361

[Arnesfield]

Hi @mark-wiemer, thanks. I looked through the changes and I think I'm fine with them. This is probably just me being subjective and nitpicky, but some things I'd like to point out:

1. The globPaths.reduce() change could probably be simplified into a flatMap() call. Although, I'm guessing the idea behind this change was to reduce the nested loops? I'm alright with that. Though, it does create an additional array and function which introduces some overhead, albeit probably negligible.
2. The Array.from(pattern.globs).some() change in the matchPattern() function I'm also fine with, just that it kinda creates an additional array and function whenever the matchPattern() call reaches that point (which can be called for every watched file change).
3. I noticed the functions createPathFilter() and createPathMatcher() were added to exports? Honestly, I'm still okay with that, but I think these functions should be kept internal to the file unless it's actually needed somewhere else.

Well, that's just me trying to optimize the little things, hope you don't mind.

I am a bit curious about the performance and what causes the ~5-second delay. Not sure when I'll have time to look into that though. Seems interesting nonetheless.

---

On another topic, something is kinda bothering me with the changes I've made. It's a small edge case that I've thought about regarding the directory globs and it's not even really an issue because of how it's written. Right now, if we have a watchFile like src/package*/src/**/*.ts, the resulting directory paths to match (parsed by createPathFilter()) would be src and src/**/* (meaning that src and all directory paths under it would not be ignored by the chokidar ignored match function, so a directory path like src/utils wouldn't be ignored).

I think the directory patterns to match (if we're using src/package*/src/**/*.ts) should be something like:

• src
• src/package*
• src/package*/src
• src/package*/src/**/*

This way, paths like src/utils would be ignored by the chokidar ignored match function.

It's a small thing that doesn't really affect how it's intended to work, since the allowed patterns wouldn't allow src/utils and its contents anyway.

I'm probably just looking into it too deeply. Let me know should we want to handle this case too, though it probably works just fine as it is now without going into too much detail.

[mark-wiemer]

Thanks @Arnesfield. Perf is definitely important here but I focused on readability first, hence the change to reduce and some. I didn't notice any significant perf decrease with those changes, so I think we should leave them for now and come back to them once we have more data about the main bottlenecks here.

Regarding adding these functions to exports, yeah, let me revert that change. Originally I was going to write unit tests specifically for those functions but I never did that, so that change has no impact other than confusion!

Regarding your edge case concern, are you able to write a test case that reproduces this? If the behavior in your PR is significantly different than Mocha 11.2.1 (with Chokidar 3) or our current code (without your changes), it's definitely important to document. With so many users of Mocha, any potential breaking change should be deeply investigated.

That said, for the record, Mocha 11.2.2 introduces a behavioral change where --watch-files [ab].js no longer watches a file literally named [ab].js, only files named a.js and b.js. This matches our documentation (which says the arg is a glob) and if users do want to watch a file of that name, they can pass in [[]ab[]].js as the arg instead. But I added a test for clarity to reduce the chance of regression.

[Arnesfield]

Looks good, thanks. For the edge case I brought up, the behavior of the watch mode wouldn't change even if it were to be addressed. It only ever applies when the watchFiles array includes a regexp before a globstar (e.g., src/reg*exp/**). It would be a kind of small, internal improvement rather than something higher up that we can test.

[mark-wiemer]

@Arnesfield gotcha, so that edge case indicates a potential perf improvement and refactor, not a potential bug in the behavior that anyone using Mocha would see?

[Arnesfield]

Yeah. Actually, maybe "improvement" isn't the right term and more like handling that edge case makes it more precise instead of being just good enough. Now that I think about it, it's hard to say if there'd be any performance improvements since being more precise with the checks means more patterns to match.

Anyway, I only brought it up so I wouldn't be the only one that's aware of it, and it could become something worth looking into in the future if it ever becomes an issue (probably not). For now, I'd say the current checks should be good enough.

Let me know should there be anything to do on my end. Thanks!

Edit: Meant precise and not concise*

[mark-wiemer]

Thanks @Arnesfield. Just FYI, we're marking this one as semver-major, but Mocha 12 is planned soon (#5357). The likelihood of breaks with this change is just too high.