fix(ext/node): support encoding option in fs.readdir; don't double-call glob callback#33501
Conversation
…ll glob callback
Two unrelated fs alignments:
- fs.readdir / fs.readdirSync: accept a string `options` argument as
shorthand for `{ encoding }` (matches getOptions() in Node's
lib/internal/fs/utils.js), validate encodings via Buffer.isEncoding,
and route filename re-encoding through Buffer.toString(encoding) so
encodings like "hex", "base64", and "buffer" actually work. Previously
TextDecoder was used and silently produced wrong output for non-text
encodings, and the string-as-encoding form was dropped on the floor.
- fs.glob: dispatch via Promise.then(onFulfilled, onRejected) instead of
awaiting inside a try/catch. Previously, if the user-supplied callback
threw on success, the catch would invoke it again with the thrown
error - causing a "called more than expected" failure. Matches Node's
lib/fs.js glob().
Enables parallel/test-fs-buffer.js and parallel/test-fs-glob-throw.mjs
in the node_compat suite.
nathanwhitbot
left a comment
nathanwhitbot
left a comment
There was a problem hiding this comment.
Reviewed against Node source. Both changes are faithful:
fs.glob (_fs_glob.ts): exact match to Node lib/fs.js:3210-3214 — PromisePrototypeThen(ArrayFromAsync(...), (res) => callback(null, res), callback). The Promise.then(onFulfilled, onRejected) shape is the textbook fix for the "callback throws → catch re-invokes it" double-call: the onRejected slot only fires for promise rejection, not for synchronous throws inside onFulfilled. Test test-fs-glob-throw.mjs directly exercises that path.
fs.readdir / readdirSync (_fs_readdir.ts):
normalizeOptions: matches Nodelib/internal/fs/utils.js:317-326getOptions()— string options promoted to{ encoding: <string> }.validateEncoding: matches Nodelib/internal/fs/utils.js:330-331+assertEncoding— skip on undefined and on'buffer', else requireBuffer.isEncoding. Note:'buffer'is intentionally not inBuffer.isEncoding's set, so this avoids a spurious throw.- New
decode():Buffer.from(str, 'utf8').toString(encoding)correctly mirrors Node's binding-level encoding (the source filename is UTF-8 from Deno'sreaddir; converting to a Buffer then re-stringifying is equivalent to what Node'sStringBytes::Encodedoes after libuv hands back UTF-8 bytes). The'buffer'branch returns a Buffer instance, matching Node'sBuffer-encoding output. - Order swap:
decode()now applied to the final relative path (afterrelative()) — matches Node'shandleFilePathswhich encodes after path resolution. ✓
Minor: the ERR_INVALID_OPT_VALUE_ENCODING thrown as a plain Error (rather than Node's ERR_INVALID_ARG_VALUE) is a pre-existing inconsistency this PR preserves — non-blocking.
Both new test entries are alphabetically placed correctly. LGTM.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Two clear, small fixes against concrete failure modes:
glob double-call: switching from try { await ... callback(null, res) } catch { callback(err) } to Array.fromAsync(...).then(res => callback(null, res), callback) is the right shape. In the old form, if the user-supplied callback threw inside the success path, the catch would re-fire it with the throw — test-fs-glob-throw.mjs exercises exactly this. With .then(onFulfilled, onRejected), a throw from onFulfilled becomes an unhandled rejection rather than re-entering onRejected, matching Node's lib/fs.js glob().
fs.readdir encoding: the bare-string options form (fs.readdir(dir, 'hex', cb)) was getting silently dropped because the old code did typeof optionsOrCallback === "object" and a string fell to null. The new normalizeOptions mirrors getOptions() in Node's lib/internal/fs/utils.js. Re-encoding via Buffer.from(name, 'utf8').toString(encoding) instead of TextDecoder is the right call — TextDecoder doesn't support hex/base64/base64url/buffer, which is why those encodings previously round-tripped to plain UTF-8. Subtle but important: moving decode(...) to after the relative(path, join(current, name)) step in the recursive branch correctly applies the encoding to the full relative path, not just the trailing filename — that's what Node does and the old code got it wrong.
One inline note about the type signatures.
| path: string | Buffer | URL, | ||
| options?: { withFileTypes?: false; encoding?: string }, | ||
| options?: { withFileTypes?: false; encoding?: string } | string, | ||
| ): string[]; |
There was a problem hiding this comment.
Type signatures don't reflect the new 'buffer' encoding path. readdirSync(path, 'buffer') now actually returns Buffer[] at runtime (via decode(...) returning a Buffer), but the matching overload picks the string[] signature here. Same on the async readdir. Worth either adding a Buffer[] overload (Node's @types/node has Buffer[] for BufferEncodingOption) or narrowing the encoding param so a literal 'buffer' resolves to a Buffer[] return — otherwise a TS user with readdirSync(p, 'buffer') will get inferred string[] and .toString('hex') won't be available without a cast.
4 participants
Two unrelated
node:fsalignments.fs.readdir / fs.readdirSync
optionsargument as shorthand for{ encoding }(matchesgetOptions()in Node'slib/internal/fs/utils.js). Previously the string form was dropped on the floor, sofs.readdir(dir, 'hex', cb)returned plain UTF-8 names.Buffer.isEncoding.Buffer.from(name, 'utf8').toString(encoding)instead ofTextDecoder(encoding)(TextDecoder doesn't supporthex/base64/base64url/buffer). Adds a'buffer'branch that returnsBufferinstances.fs.glob
Dispatch via
Promise.then(onFulfilled, onRejected)rather than awaiting inside atry/catch. Previously, if the user-supplied callback threw on success, the catch would invoke it again with the thrown error — causing a "called more than expected" failure. Matches Node'slib/fs.jsglob().Tests enabled
parallel/test-fs-buffer.jsparallel/test-fs-glob-throw.mjsVerified no regressions across the configured
test-fs-readdir-*andtest-fs-glob*tests.