fix: watch future glob matches without mutating options - #5731
fix: watch future glob matches without mutating options#5731OskarEichler wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. Walkthrough
Merge Risk: 🔵 Low · up to The watch behavior may still mishandle absolute root-level globs and negated dynamic ignore patterns, which could cause matching files to be missed or incorrectly included during live reload. The change is mergeable with explicit owner awareness and follow-up on these bounded path-filtering cases. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
| "launch-editor": "^2.14.1", | ||
| "open": "^11.0.0", | ||
| "p-retry": "^8.0.0", | ||
| "picomatch": "^4.0.4", |
There was a problem hiding this comment.
We should avoid extra packages here
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
We don't need multiple glob related packages
🦋 Changeset detectedLatest commit: fda6370 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: fb40762e-ab2b-40e4-9a26-d895395863ea
📒 Files selected for processing (1)
lib/Server.js
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| const base = prefix.endsWith("/") | ||
| ? prefix.slice(0, -1) | ||
| : path.posix.dirname(prefix); | ||
|
|
||
| const expand = (/** @type {string} */ item) => { | ||
| const posix = toPosix(item); | ||
| return isDynamicPattern(posix) | ||
| ? globSync(posix, { cwd, absolute: true }).map(toNative) | ||
| : item; | ||
| return path.resolve(cwd, (base || ".").replace(/\\(.)/g, "$1")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the filesystem root in getWatchRoot.
For an absolute glob such as /*, prefix is /. This branch removes the only separator, sets base to ., and returns cwd instead of /. Chokidar then watches the wrong root, so the absolute glob misses its intended matches. Preserve platform root prefixes before resolving the watch root.
🧰 Tools
🪛 ESLint
[error] 3457-3457: Prefer String#replaceAll() over String#replace().
(unicorn/prefer-string-replace-all)
| ? [] | ||
| : [originalIgnored]) { | ||
| if (typeof item === "string" && isDynamicPattern(toPosix(item))) { | ||
| ignored.push((file) => matchesGlob(file, toPosix(item))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Server.js relevant implementation ---'
sed -n '3380,3535p' lib/Server.js
printf '%s\n' '--- package/runtime declarations ---'
if [ -f package.json ]; then
sed -n '1,180p' package.json
fi
printf '%s\n' '--- relevant symbols and path API usage ---'
rg -n -C 3 'watchFiles|watchOptions|matchesGlob|getWatchRoot|ignored|toPosix|anymatch|picomatch' lib package.json test tests 2>/dev/null | head -n 500Repository: webpack/webpack-dev-server
Length of output: 40011
🌐 Web query:
Node.js v22.15.0 path.matchesGlob negation leading ! contract and Chokidar 5 ignored anymatch picomatch string matcher negation
💡 Result:
Node.js path.matchesGlob(path, pattern) does not natively support glob negation using a leading ! character [1][2][3]. As of Node.js v22.15.0, path.matchesGlob is an experimental API (marked stable in later versions) [2][4] designed to test whether a given string path matches a provided glob pattern string [1][4]. Its implementation expects a standard glob pattern and does not interpret leading ! as a negation operator [1][4]. Regarding Chokidar 5 and negation in ignored patterns: 1. Chokidar relies on anymatch to process its ignored option [5][6]. 2. Historically, while anymatch has supported negation patterns (e.g., ['/node_modules/', '!/node_modules/important/']), using them in Chokidar can be problematic because Chokidar performs an initial check against the parent path [7][6]. If a parent directory is matched by an "ignore" pattern, Chokidar often stops traversal immediately and does not evaluate subsequent "negation" (inclusion) patterns for children of that directory [7][6]. 3. For reliable results when needing to ignore most of a folder but keep specific files, the recommended approach is to provide a function to the ignored option rather than using glob strings with negation [8][9]. This allows for granular, programmatic control: javascript ignored: (path, stats) => { // Return true to ignore, false to watch if (path.includes('node_modules') && !path.endsWith('important-file.js')) { return true; } return false; } 4. Recent versions of Chokidar have emphasized using functions or regexes in the ignored field to avoid the ambiguity and limitations of glob-based negation string matching [8][9].
Citations:
- 1: https://nodejs.org/docs/v22.14.0/api/path.html
- 2: https://nodejs.org/docs/latest-v22.x/api/path.html
- 3: https://github.com/nodejs/node/blob/master/doc/api/path.md
- 4: https://nodejs.org/api/path.html
- 5: https://github.com/paulmillr/chokidar/blob/main/src/index.ts
- 6: GitHub issue 628 in paulmillr/chokidar (link omitted to avoid creating a cross-reference)
- 7: GitHub issue 1225 in paulmillr/chokidar (link omitted to avoid creating a cross-reference)
- 8: https://github.com/paulmillr/chokidar?tab=readme-ov-file
- 9: https://www.npmjs.com/package/chokidar
Preserve negation semantics for dynamic watchOptions.ignored strings.
When a dynamic ignored string starts with !, the branch at lib/Server.js:3498 passes that pattern unchanged to path.matchesGlob. path.matchesGlob treats the leading ! literally, so the matcher can return false for files that Chokidar’s anymatch/picomatch semantics would ignore. Strip and apply the negation before creating the matcher.
2 participants
Fixes
Compatibility
No API/engine removal. Adds picomatch ^4.0.4 as a direct dependency (already present transitively in the lockfile). Relative patterns remain cwd-relative; events retain chokidar's cwd semantics. Newly created matches now trigger live reload when subsequently edited. Directory watching is necessary to discover future matches; nonmatching files are excluded.
Verification
All 32 unchanged upstream watch-files tests pass. Ten real filesystem scenarios pass: absolute/cwd paths, negative patterns, function/regex/object ignores, mixed literal/glob paths, missing roots, escaped bracket paths and frozen options. Original source reproduces missed new files and option mutation. Build and full lint/types/formatting/spelling pass. Windows filesystem execution remains unverified.
Audit scope
This is a focused, independently based change from a broader source review at f804962. The combined frozen-source run executed 1,002 tests: 951 passed, 43 failed, one cancelled, seven skipped. Failures were traced to the separately proposed overlay DOM snapshots, reconnect-disabled test expectation and IPv6 host-test assumptions; it is not represented as a green full suite. Relevant focused results are listed above. No checked-in tests/specs/snapshots were added or modified. Hosted CI and the full OS/Node matrix remain pending.
Summary by CodeRabbit
New Features
Bug Fixes