Skip to content

chore: move module watcher from chokidar to fs#4240

Open
bryce-seifert wants to merge 2 commits into
bitfocus:mainfrom
bryce-seifert:fix/moduleWatcher
Open

chore: move module watcher from chokidar to fs#4240
bryce-seifert wants to merge 2 commits into
bitfocus:mainfrom
bryce-seifert:fix/moduleWatcher

Conversation

@bryce-seifert

@bryce-seifert bryce-seifert commented Jun 14, 2026

Copy link
Copy Markdown
Member

This prevents EMFILE errors (same situation as) #4068, that remerged after chokidar version update.

Summary by CodeRabbit

  • Chores
    • Updated developer-mode extra-module watching to use native Node.js file watching instead of an external dependency.
    • Improved reload triggering by filtering watched changes to relevant file types, ignoring node_modules, and debouncing per-module reloads to reduce noisy updates.
    • Added clearer error handling for module watching so watcher failures are reported more reliably.

This prevents EMFILE errors after chokidar version update
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 241560b9-4846-4b7d-a366-d16e71b73b1c

📥 Commits

Reviewing files that changed from the base of the PR and between 29c1fee and 5f9ff1e.

📒 Files selected for processing (1)
  • tools/dev.mts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tools/dev.mts

📝 Walkthrough

Walkthrough

Both launcher/main.js and tools/dev.mts drop the chokidar dependency for watching extra dev module directories and replace it with Node.js built-in fs.watch using recursive: true. The new watchers filter events by file extension (.mjs, .js, .cjs, .json), ignore node_modules paths, derive a module directory name from the first path segment, and debounce reload-extra-module IPC messages accordingly.

Changes

Migrate extra module watcher from chokidar to fs.watch

Layer / File(s) Summary
launcher/main.js: chokidar removal and fs.watch watcher
launcher/main.js
Removes the chokidar import, updates the watcher JSDoc type to FSWatcher | null, replaces chokidar.watch with fs.watch({ recursive: true, persistent: true }), filters change events by allowed extensions and excludes node_modules paths, extracts moduleDirName from the first path segment, and relocates the error handler onto the new watcher.
tools/dev.mts: fs.watch module watcher migration
tools/dev.mts
Replaces the chokidar-based watcher (previously deferred to mainWatcher ready) with an immediately-started fs.watch recursive watcher on devModulesPath, validates the path exists before watching, filters events by extension, ignores node_modules, debounces reloads per top-level directory, sends reload-extra-module IPC messages with the computed full path, and adds a dedicated error handler.

Poem

🪬 Out with the watcher from lands far away,
chokidar bows as fs.watch saves the day!
Recursive and native, persistent and keen,
filtering .mjs files right in between.
No extra deps needed — Node has it all,
just modules reloading at every file call. 🎉

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main change: replacing the chokidar module watcher with fs.watch in the launcher and dev tool files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bryce-seifert bryce-seifert requested a review from Julusian June 14, 2026 00:52

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
launcher/main.js (1)

287-313: ⚠️ Potential issue | 🟠 Major

Nice catch on the fs.FSWatcher migration — the .close().catch(...) calls need a fix to avoid runtime errors.

Line 287 switches watcher to fs.FSWatcher, but lines 448, 720, 1000, and 1067 still call watcher.close().catch(...). Since FSWatcher.close() is synchronous and returns void (not a Promise), chaining .catch() will throw a TypeError at runtime. Line 299 also awaits the close, which is semantically incorrect (though it won't error in this case).

The suggested fix with a dedicated closeWatcher() helper is solid and would clean this up nicely — we'd handle the sync close with try/catch in one place and call it consistently everywhere.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 70fcc8ed-a609-4795-b874-23d9544a7acc

📥 Commits

Reviewing files that changed from the base of the PR and between 240f180 and 29c1fee.

📒 Files selected for processing (2)
  • launcher/main.js
  • tools/dev.mts

Comment thread tools/dev.mts Outdated
@Julusian

Copy link
Copy Markdown
Member

There are quite a few documented caveats for this api, some could maybe be a problem?
https://nodejs.org/docs/latest/api/fs.html#caveats

In particular I am wondering about:

If the underlying functionality is not available for some reason, then fs.watch() will not be able to function and may throw an exception. For example, [...] or host file systems when using virtualization software such as Vagrant or Docker.

I wonder what that means for WSL, as that is a virtual machine these days, or if anyone is using something like devcontainers on macos

@bryce-seifert

Copy link
Copy Markdown
Member Author

Good points / questions. I'll need to do some reading on those use cases, as I'm not entirely sure / have not tested outside of a Mac dev environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants