Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(deps): update chokidar to v4 #5256

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore(deps): update chokidar to v4 #5256

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6893764
chore(deps): update choikdar to v4
ggodlewski Nov 16, 2024
90d7cce
Merge branch 'mochajs:main' into main
ggodlewski Dec 4, 2024
65eee8d
chore: resolve comment issues
ggodlewski Jan 2, 2025
d214cda
fix: prevent traversing ignored directories
ggodlewski Jan 30, 2025
f9fa7e8
Remove unnecessary line
ggodlewski Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 49 additions & 4 deletions lib/cli/watch-run.js
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] I tried this PR out on real-world code and found a performance issue from files being renamed. I found it on the ESLint codebase but also reproduced it in isolation here: https://github.com/JoshuaKGoldberg/repros/tree/mocha-chokidar-v4-pr-watching. Could you please take a look?

tl;dr: in a minimal codebase with two root-level test files, re-watching is reported as taking >200ms

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly are you renaming the file? I run mv b.test.js z.test.js and I don't have the state with 0 passing:

$ node ../mocha/bin/mocha.js "*.test.js" --watch
  a
    ✔ passes
  b
    ✔ passes
  2 passing (2ms)

ℹ [mocha] waiting for changes...

  a
    ✔ passes
  b
    ✔ passes

  2 passing (1ms)

ℹ [mocha] waiting for changes...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I did it through the VS Code UI. This is on an m1 Mac.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm running Ubuntu. I have no hardware to reproduce it. It's strange because the tracker.regenerate(); is only triggered on event === 'add'. I don't see how the list of files can be empty during rename of 1 file. Maybe you can add some debug output so we can trace the tracker method calls?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 added to the README.md. Interestingly, the long delay of ~200ms only happens if an npm install populated node_modules/. If you rm -rf node_modules/ then the delay is only around 20-30ms.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one more improvement. Glob should not traverse node_modules. Can you check if it improves anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 that fixed it for me! Back to 0-1ms on the middle waiting.

+1 that it's a generalized solution too, not specific to node_modules. Nice 🙂

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const path = require('path');
const chokidar = require('chokidar');
const Context = require('../context');
const collectFiles = require('./collect-files');
const glob = require('glob');

/**
* Exports the `watchRun` function that runs mocha in "watch" mode.
Expand Down Expand Up @@ -136,6 +137,43 @@ exports.watchRun = (mocha, {watchFiles, watchIgnore}, fileCollectParams) => {
});
};

class GlobFilesTracker {
constructor(watchFiles, watchIgnore) {
this.watchFilesSet = new Set();
this.watchFiles = watchFiles;
this.watchIgnore = watchIgnore;
}

regenerate() {
const watchIgnoreSet = new Set();
for (const pattern of this.watchIgnore) {
glob.sync(pattern, { dot: true }).forEach(filePath => watchIgnoreSet.add(filePath));
}

const globOpts = {
dot: true,
ignore: {
ignored: () => false,
ggodlewski marked this conversation as resolved.
Show resolved Hide resolved
childrenIgnored: pathToCheck => watchIgnoreSet.has(pathToCheck.relative())
}
};

this.watchFilesSet.clear();
for (const pattern of this.watchFiles) {
glob.sync(pattern, globOpts).forEach(pathToCheck => {
if (watchIgnoreSet.has(pathToCheck)) {
return;
}
this.watchFilesSet.add(pathToCheck);
});
}
}

has(filePath) {
return this.watchFilesSet.has(filePath)
}
}

/**
* Bootstraps a chokidar watcher. Handles keyboard input & signals
* @param {Mocha} mocha - Mocha instance
Expand Down Expand Up @@ -167,8 +205,10 @@ const createWatcher = (
// we handle global fixtures manually
mocha.enableGlobalSetup(false).enableGlobalTeardown(false);

const watcher = chokidar.watch(watchFiles, {
ignored: watchIgnore,
const tracker = new GlobFilesTracker(watchFiles, watchIgnore);
tracker.regenerate();

const watcher = chokidar.watch('.', {
ignoreInitial: true
});

Expand All @@ -184,8 +224,13 @@ const createWatcher = (
rerunner.run();
});

watcher.on('all', () => {
rerunner.scheduleRun();
watcher.on('all', (event, filePath) => {
if (event === 'add') {
tracker.regenerate();
}
if (tracker.has(filePath)) {
rerunner.scheduleRun();
}
});

hideCursor();
Expand Down
Loading