fix(ext/node): support ignore option in fs.watch and fs.promises.watch#33610
fix(ext/node): support ignore option in fs.watch and fs.promises.watch#33610bartlomieju merged 11 commits intodenoland:mainfrom
ignore option in fs.watch and fs.promises.watch#33610Conversation
….watch` Implement the `ignore` option for `fs.watch` (sync) and `fs.promises.watch` (async iterable). Accepts a string (minimatch glob), `RegExp`, function, or array of those. Watch events whose filename matches any pattern are filtered out. Mirrors Node's `createIgnoreMatcher` / `validateIgnoreOption` from https://github.com/nodejs/node/blob/main/lib/internal/fs/watchers.js, reusing the minimatch dep already vendored at `ext:deno_node/deps/minimatch.js`. Enables 14 node compat tests: - `test-fs-promises-watch-ignore-function.mjs` - `test-fs-promises-watch-ignore-glob.mjs` - `test-fs-promises-watch-ignore-invalid.mjs` - `test-fs-promises-watch-ignore-mixed.mjs` - `test-fs-promises-watch-ignore-regexp.mjs` - `test-fs-watch-ignore-function.js` - `test-fs-watch-ignore-glob.js` - `test-fs-watch-ignore-invalid.js` - `test-fs-watch-ignore-mixed.js` - `test-fs-watch-ignore-recursive-glob.js` - `test-fs-watch-ignore-recursive-glob-subdirectories.js` - `test-fs-watch-ignore-recursive-mixed.js` - `test-fs-watch-ignore-recursive-regexp.js` - `test-fs-watch-ignore-regexp.js` Supersedes denoland#33606 (which only covered the promise version).
fibibot
left a comment
There was a problem hiding this comment.
The unified scope (sync fs.watch + async fs.promises.watch in one PR) is a sensible consolidation. The validators correctly mirror Node's validateIgnoreOption / validateIgnoreOptionElement, createIgnoreMatcher builds an OR'd list of compiled matchers, and the watch() / watchPromise() call sites both call the validator and apply the matcher to filter events. Lazy-loading minimatch via core.createLazyLoader keeps non-glob callers off the dep cost.
But CI lint is red on the same prefer-primordials pattern that already came up in #33574 and #33606 — same four hits:
- L3161
value instanceof RegExp→ObjectPrototypeIsPrototypeOf(RegExpPrototype, value) - L3172
Array.isArray(value)→ArrayIsArray(value) - L3185
Array.isArray(ignore)→ArrayIsArray(ignore) - L3204
matcher instanceof RegExp→ObjectPrototypeIsPrototypeOf(RegExpPrototype, matcher)
Same fix as both prior PRs — add RegExpPrototype and ArrayIsArray to the existing primordials destructure (ObjectPrototypeIsPrototypeOf is already imported). Cribbing #33574's resolution commit verbatim should land this clean.
Inline detail. Note also: this PR overlaps with #33574 (which is approved but not yet merged) — closing #33574 in favor of this consolidated version is the right call as long as #33574's enrolled test-fs-watch-ignore-invalid.js is included here. Looks like it's not in the 14-test list — worth checking that you're not regressing test coverage from #33574 → #33610.
nathanwhitbot
left a comment
There was a problem hiding this comment.
A few blockers:
Lint failures (CI lint is red on linux/windows/macos). Same pattern I hit on #33574 — primordials are required:
value instanceof RegExp(lines 3161, 3204) — needsObjectPrototypeIsPrototypeOf(RegExpPrototype, value)matcher instanceof RegExp— same fixArray.isArray(value)(line 3172, 3185) — needsArrayIsArray(value)mm.match(filename)is fine butmatcher.test(filename)should useRegExpPrototypeTest(matcher, filename)to match Node's actualcreateIgnoreMatcherlet _lazyMinimatch: any = null;—anyis rejected by lint
You'll need to add ArrayIsArray, RegExpPrototype, RegExpPrototypeTest, ObjectPrototypeIsPrototypeOf to the primordials destructure at the top.
Behavior difference vs Node: Node's createIgnoreMatcher (lib/internal/fs/watchers.js) compiles strings via globStringToRegExp, not minimatch. Setting matchBase: true makes *.log match every nested basename, which is convenient but is not what Node does — Node-derived globStringToRegExp requires the pattern to match the filename argument as-is. The test-fs-watch-ignore-glob* tests pass per your test plan, but worth a sanity check that the matchers behave identically on a tree where a top-level pattern shouldn't match a nested path.
Replaces `instanceof RegExp` with `ObjectPrototypeIsPrototypeOf(RegExpPrototype, ...)`, `Array.isArray` with `ArrayIsArray`, RegExp.test with `RegExpPrototypeTest`, and `Array.push` with `ArrayPrototypePush`, plus the `any` lint-ignore on the lazy-loader cell.
fibibot
left a comment
There was a problem hiding this comment.
LGTM after d5049d7a. All four prefer-primordials hits fixed exactly as suggested:
value instanceof RegExp/matcher instanceof RegExp→ObjectPrototypeIsPrototypeOf(RegExpPrototype, ...)Array.isArray(value)/Array.isArray(ignore)→ArrayIsArray(...)
Bonus primordials hygiene that wasn't even flagged: compiled.push(...) → ArrayPrototypePush(compiled, ...) and matcher.test(filename) → RegExpPrototypeTest(matcher, filename). ArrayIsArray, RegExpPrototype, RegExpPrototypeTest correctly added to the existing primordials destructure.
Also walking back my prior test-coverage concern — test-fs-watch-ignore-invalid.js from #33574 IS enrolled here (line 1199 in the diff), under the sync test-fs-watch-ignore-* block. I miscounted in the prior review.
CI now satisfies the gate cleanly: all 3 lint debug platforms green, all 4 platforms × 3 shards × debug+release of test node_compat green (15/15 of those green), test unit_node debug green where it ran. 108/123 checks pass, 0 fail. Remaining pending jobs are unrelated macos-x86_64 specs and a release build — none affecting the fs.watch ignore path.
(license/cla is showing pending — appears to be an automation lag; @nathanwhitbot is presumably already CLA-cleared from prior PRs, will resolve on its own.)
d5049d7 to
012384b
Compare
Summary
Implement the
ignoreoption for bothfs.watch(sync) andfs.promises.watch(async iterable). Accepts a string (minimatch glob),RegExp, function, or array of those — watch events whose filename matches any pattern are filtered out. Mirrors Node'screateIgnoreMatcher/validateIgnoreOption, reusing the minimatch dep already vendored atext:deno_node/deps/minimatch.js.Enables 14 node compat tests:
test-fs-promises-watch-ignore-{function,glob,invalid,mixed,regexp}(5)test-fs-watch-ignore-{function,glob,invalid,mixed,regexp}(5)test-fs-watch-ignore-recursive-{glob,glob-subdirectories,mixed,regexp}(4)Supersedes #33606 (which only covered the promise version).
Test plan
cargo test --test node_compat -- watch-ignoretest-fs-watch.js,test-fs-promises-watch-iterator.js,test-fs-watch-ref-unref.js,test-fs-watch-stop-{async,sync}.js,test-fs-watch-recursive-add-file-to-existing-subfolder.js,test-fs-watch-recursive-delete.js)