fix(ext/node): return relative path from fs.watch with recursive option#33428
Conversation
fs.watch() with { recursive: true } was always passing basename() of
the changed file path to the callback. Node.js returns the relative
path from the watched directory for recursive watches (e.g.
"subdir/file.txt" instead of "file.txt"). Use path.relative() for
recursive watches and basename() for non-recursive, matching Node.
Closes #33419
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
The fix in fs.ts is right — relative(resolvedWatchPath, val.paths[0]) for recursive watches is what Node does, and applying it to both watch() and watchPromise() keeps the two surfaces consistent. The realpathSync(watchPath) step is also necessary because Deno.watchFs returns symlink-resolved paths.
The blocker is the new unit test on macOS: [node/fs] watch recursive returns relative path for nested files fails with Expected relative path like "sub/test.txt", got: sub. That's not a bug in the polyfill — macOS FSEvents (which Deno.watchFs's recursive mode is built on) only delivers directory-granularity events, so val.paths[0] is /tmp/…/sub and the new relative() call correctly produces "sub". Node behaves the same way on macOS for the same reason.
Easiest fix is to either (a) add Deno.build.os === "darwin" to the existing ignore, (b) loosen the assertion to f.startsWith("sub") so the directory-level event still passes, or (c) swap the test to a non-recursive watch on subdir directly, where Linux/macOS both produce test.txt.
|
|
||
| Deno.test({ | ||
| name: "[node/fs] watch recursive returns relative path for nested files", | ||
| ignore: Deno.build.os === "windows", |
There was a problem hiding this comment.
Add || Deno.build.os === "darwin" here, or convert to unsupported: { darwin: ..., windows: ... } — macOS recursive watches use FSEvents which only fires directory-granularity events, so val.paths[0] is the parent dir and relative() returns "sub", not "sub/test.txt".
| watcher.close(); | ||
|
|
||
| const hasRelative = filenames.some((f) => | ||
| f === join("sub", "test.txt") || f === "sub/test.txt" |
There was a problem hiding this comment.
If you want this test to also exercise the macOS path, replace the equality check with f.startsWith("sub") or f === "sub" || f === "sub/test.txt". Today macOS receives only "sub" (which is the correct relative path of the directory event the OS gave us), so the assertion fails CI.
macOS FSEvents only delivers directory-granularity events for recursive
watches, so the filename is "sub" rather than "sub/test.txt". Use
startsWith("sub") to accept both.
Keep both the watch recursive test (this PR) and the readFile AbortSignal test (from main).
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. The follow-up commit 2c92efeb ("fix(test): loosen fs.watch recursive assertion for macOS FSEvents") addresses the prior CHANGES_REQUESTED exactly:
- Assertion changed from strict
f === "sub/test.txt"tof.startsWith("sub")so macOS's directory-granularity FSEvents ("sub") and Linux/Windows's file-granularity events ("sub/test.txt") both pass. - Added a comment justifying the looser check, anchored to the FSEvents behaviour.
The substantive fix in fs.ts (relative(resolvedWatchPath, val.paths[0]) for recursive watches in both watch() and watchPromise(), with realpathSync to handle the /var → /private/var case) is unchanged from the prior pass. The follow-up merge commit (50283297) is just resolving the trivial fs_test.ts conflict against main. CI is rerunning with 0 failures so far.
2 participants
Summary
Closes #33419
fs.watch()with{ recursive: true }was always passingbasename()of the changed file path to the callback. Node.js returns the relative path from the watched directory for recursive watches (e.g."subdir/file.txt"instead of just"file.txt").Fix: use
path.relative(watchedDir, changedPath)for recursive watches andbasename()for non-recursive, matching Node.js behavior. Applied to both the callback-stylefs.watch()and the async iterablefs.promises.watch().Test plan
Before:
After: