Skip to content

fix(ext/node): support encoding option in fs.watch#33634

Open
nathanwhitbot wants to merge 2 commits intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter58
Open

fix(ext/node): support encoding option in fs.watch#33634
nathanwhitbot wants to merge 2 commits intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter58

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

fs.watch(path, { encoding }) previously emitted the filename as a plain utf8 string regardless of the requested encoding. Match Node's lib/internal/fs/watchers.js:

  • encoding: 'buffer' → emit a Buffer
  • any other named encoding (e.g. 'hex', 'base64') → re-encode the filename via Buffer.from(filename).toString(encoding)
  • default ('utf8' or absent) → leave the string unchanged

Enables parallel/test-fs-watch-encoding.js.

Test plan

  • parallel/test-fs-watch-encoding.js passes via cargo test --test node_compat
  • Existing fs-watch*/fs-promises-watch* tests still pass

`fs.watch(path, { encoding })` previously emitted the filename as a plain
utf8 string regardless of the requested encoding. Match Node:
- `encoding: 'buffer'` returns a `Buffer`,
- any other named encoding (e.g. `'hex'`, `'base64'`) re-encodes the
  filename via `Buffer.from(filename).toString(encoding)`,
- the default (`'utf8'` or absent) leaves the string unchanged.

See https://github.com/nodejs/node/blob/main/lib/internal/fs/watchers.js

Enables `parallel/test-fs-watch-encoding.js`.
Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

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

Encoding paths line up with Node's behavior — utf8/undefined pass-through, 'buffer' returns a Buffer, other named encodings via Buffer.from(filename).toString(encoding). Net effect is the same as Node's Buffer.from(<os-bytes>).toString(encoding): the original UTF-8 bytes get re-interpreted through the target encoding, including the same lossy behavior for utf16le/ucs2 on odd-byte filenames.

The watchListener type is also updated to string | Buffer, matching Node's typings.

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 substance is right and the helper is concise. Verified against the upstream test:

  • encoding: 'hex' for filename 新建文夹件.txt: Buffer.from(filename).toString('hex')'e696b0e5bbbae69687e5a4b9e4bbb62e747874' (UTF-8 bytes hex-encoded), which is exactly the value the test asserts.
  • No encoding (or 'utf8'): early-return the original string ✓.
  • encoding: 'buffer': Buffer.from(filename) → real Buffer instance, satisfies the test's filename instanceof Buffer && filename.toString('utf8') === fn check ✓.

Buffer (line 92) and BufferEncoding (line 102) are already imported in fs.ts, so no new imports. watchListener type widening to (eventType, string | Buffer) => void correctly reflects the new return shape.

Test enrollment alphabetically positioned correctly (fs-utimes < fs-watch-encoding < fs-watch-file-enoent-after-deletion).

Holding off on autonomous APPROVE per fibibot's CI gate — only license/cla and lint title have run so far. The full CI build needs maintainer authorization to run on this bot's PRs. I'll re-confirm once test node_compat debug and test unit_node debug land green.

One small nit: passing an unknown encoding (e.g., { encoding: 'foo' }) currently falls through to asBuffer.toString('foo' as BufferEncoding) which Buffer will throw on at call time. Node validates the encoding upfront and throws ERR_INVALID_ARG_VALUE synchronously. Not a functional difference for the test, but validateEncoding would surface a cleaner error. Non-blocking.

@bartlomieju
Copy link
Copy Markdown
Member

@nathanwhitbot this needs a rebase

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

test node_compat (1/3) debug macos-x86_64 failed on test-process-threadCpuUsage-worker-threads.js — same macOS x86_64 timing flake we've seen on #33559 / #33601. That test isn't part of this PR's changes (it's already enabled in main). cc @nathanwhit

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.

4 participants