Skip to content

fix(ext/node): support ignore option in fs.promises.watch#33606

Closed
nathanwhitbot wants to merge 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter52-v2
Closed

fix(ext/node): support ignore option in fs.promises.watch#33606
nathanwhitbot wants to merge 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter52-v2

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

Implement the ignore option for fs.promises.watch, accepting a string (minimatch glob), RegExp, function, or array of those. Watch events whose filename matches any pattern are filtered out. Mirrors Node's createIgnoreMatcher and validateIgnoreOption, reusing the minimatch dep already vendored at ext:deno_node/deps/minimatch.js.

Enables 5 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 plan

  • All 5 ignore tests pass via cargo test --test node_compat -- fs-promises-watch-ignore
  • Existing fs-watch* tests still pass (test-fs-promises-watch-iterator.js, test-fs-watch.js, test-fs-watch-ref-unref.js)

Implement the `ignore` option for `fs.promises.watch`, accepting a string
(minimatch glob), RegExp, function, or array of those. Filters watch events
whose filename matches any pattern. Mirrors Node's `createIgnoreMatcher` /
`validateIgnoreOption` from
https://github.com/nodejs/node/blob/main/lib/internal/fs/watchers.js.

Reuses the minimatch dep already vendored at `ext:deno_node/deps/minimatch.js`.

Enables 5 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`
Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

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

The implementation is solid — validateIgnoreOption / validateIgnoreOptionElement cleanly mirror Node's lib/internal/validators.js (string-or-RegExp-or-function with empty-string ERR_INVALID_ARG_VALUE, array recursion, null/undefined no-op). createIgnoreMatcher builds an OR'd list of matchers and the while (true) await fsIterable.next() loop in watchPromise correctly skips events the matcher rejects without losing positional state. Reusing the already-vendored minimatch.js via core.createLazyLoader is the right move — keeps the import lazy so non-glob callers don't pay for it.

But CI is red on prefer-primordials lint across all 3 platforms — same pattern as #33574 had:

  • value instanceof RegExp (line 3252)
  • Array.isArray(value) (line 3263)
  • Array.isArray(ignore) (line 3276, in createIgnoreMatcher)
  • matcher instanceof RegExp (line 3295, in createIgnoreMatcher)

The fix is mechanical. Inline guidance below.

(Probably copied from #33574 before that PR's primordials fix landed — the fix is identical here.)

Comment thread ext/node/polyfills/fs.ts
}
return;
}
if (value instanceof RegExp) return;
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.

Lint blocker — prefer-primordials rejects all four hits in this PR:

  • L3252: value instanceof RegExpObjectPrototypeIsPrototypeOf(RegExpPrototype, value)
  • L3263: Array.isArray(value)ArrayIsArray(value)
  • L3276: Array.isArray(ignore)ArrayIsArray(ignore)
  • L3295: matcher instanceof RegExpObjectPrototypeIsPrototypeOf(RegExpPrototype, matcher)

The file already imports ObjectPrototypeIsPrototypeOf from primordials at line 174. After #33574 landed, RegExpPrototype and ArrayIsArray should also already be in the destructure — if not, add them.

Full failure: https://github.com/denoland/deno/actions/runs/25018807704/job/73273702258

#33574 had the exact same pattern fixed in the same way; you can crib that fix verbatim.

Comment thread ext/node/polyfills/fs.ts
| RegExp
| ((filename: string) => boolean)
| (string | RegExp | ((filename: string) => boolean))[]
| undefined
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.

Minor — let _lazyMinimatch: any = null is using any for the lazy-loader handle. This is contagious: _lazyMinimatch ??= core.createLazyLoader(...) then _lazyMinimatch() returns any, and getMinimatch().default is therefore any too.

Not blocking, but typing it as something narrower (e.g. () => typeof import("...") or just () => unknown and then asserting at the destructure site) would catch a mistyped property access at compile time. The current shape would silently break if Minimatch got renamed in the dep.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Superseded by #33610 which adds the same fix for both fs.watch and fs.promises.watch (this PR only covered fs.promises.watch).

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.

3 participants