Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/js/node/fs.promises.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,20 @@ const exports = {
},
read: asyncWrap(fs.read, "read"),
write: asyncWrap(fs.write, "write"),
readdir: asyncWrap(fs.readdir, "readdir"),
readdir: async function readdir(path, options) {
// Unlike fs.readdir, node's promise form only tests `options.recursive` for
// truthiness, while the shared native parser implements fs.readdir's check.
// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/promises.js#L1601
Comment thread
robobun marked this conversation as resolved.
Outdated
if (typeof options === "object" && options !== null) {
const { recursive } = options;
if (recursive != null && typeof recursive !== "boolean") {
// The native parser reads the other options through the prototype chain,
// so it sees exactly what it would have seen on the caller's object.
Comment thread
robobun marked this conversation as resolved.
Outdated
options = { __proto__: options, recursive: !!recursive };
}
Comment thread
claude[bot] marked this conversation as resolved.
}
return fs.readdir(path, options);
},
readFile: async function (fileHandleOrFdOrPath, ...args) {
fileHandleOrFdOrPath = fileHandleOrFdOrPath?.[kFd] ?? fileHandleOrFdOrPath;
return _readFile(fileHandleOrFdOrPath, ...args);
Expand Down
14 changes: 11 additions & 3 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3485,10 +3485,18 @@ pub mod args {
_ => {
if val.is_object() {
encoding = get_encoding(val, ctx, encoding)?;
if let Some(r) = val.get_boolean_strict(ctx, "recursive")? {
recursive = r;
// readdir/readdirSync skip the boolean check for a nullish
// `recursive` (fs.promises.readdir coerces it in fs.promises.ts):
// https://github.com/nodejs/node/blob/v26.3.0/lib/fs.js#L1546-L1548
Comment thread
robobun marked this conversation as resolved.
Outdated
if let Some(r) = val.get(ctx, "recursive")? {
if !r.is_null() {
recursive = validators::validate_boolean(ctx, r, "recursive")?;
}
}
if let Some(w) = val.get_boolean_strict(ctx, "withFileTypes")? {
// Node never validates `withFileTypes`; every entry point
// passes `!!options.withFileTypes`:
// https://github.com/nodejs/node/blob/v26.3.0/lib/fs.js#L1557
Comment thread
robobun marked this conversation as resolved.
Outdated
if let Some(w) = val.get_boolean_loose(ctx, "withFileTypes")? {
with_file_types = w;
}
}
Expand Down
100 changes: 99 additions & 1 deletion test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { beforeAll, describe, expect, it, spyOn } from "bun:test";
import { beforeAll, describe, expect, it, jest, spyOn } from "bun:test";
import {
bunEnv,
bunExe,
Expand Down Expand Up @@ -1705,6 +1705,104 @@ it("readdir with { encoding: 'buffer' } returns Buffer entries", async () => {
).toEqual(expected);
});

// readdir/readdirSync validate `options.recursive` only when it is not nullish
// and never validate `withFileTypes` (they pass `!!options.withFileTypes`);
// fs.promises.readdir only tests `recursive` for truthiness.
// https://github.com/nodejs/node/blob/v26.3.0/lib/fs.js#L1546-L1557
// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/promises.js#L1601-L1608
describe("readdir accepts the recursive/withFileTypes values node accepts", () => {
const tree = { "a.txt": "", "sub/b.txt": "" };
const flat = { names: ["a.txt", "sub"], dirents: false };
const flatDirents = { names: ["a.txt", "sub"], dirents: true };
const deep = { names: ["a.txt", "b.txt", "sub"], dirents: false };
const deepDirents = { names: ["a.txt", "b.txt", "sub"], dirents: true };

// Entry names only (the basename of a recursive path entry), so the summary
// is the same for string and Dirent results on every platform.
function summarize(entries: (string | Dirent)[]) {
const dirents = entries.every(entry => entry instanceof Dirent);
const names = entries.map(entry => (typeof entry === "string" ? entry.split(/[\\/]/).pop()! : entry.name)).sort();
return { names, dirents };
}

const readdirCallback = promisify(fs.readdir) as (path: string, options: any) => Promise<(string | Dirent)[]>;

const acceptedEverywhere: [options: Record<string, unknown>, expected: typeof flat][] = [
[{ recursive: undefined }, flat],
[{ recursive: null }, flat],
[{ recursive: true }, deep],
[{ withFileTypes: undefined }, flat],
[{ withFileTypes: null }, flat],
[{ withFileTypes: 0 }, flat],
[{ withFileTypes: "" }, flat],
[{ withFileTypes: 1 }, flatDirents],
[{ withFileTypes: "x" }, flatDirents],
[{ withFileTypes: {} }, flatDirents],
[{ withFileTypes: 1, recursive: true }, deepDirents],
[{ withFileTypes: null, recursive: null }, flat],
];

it.each(acceptedEverywhere.map(([options, expected]) => [inspect(options), options, expected] as const))(
"readdirSync, readdir and promises.readdir accept %s",
async (_name, options, expected) => {
using dir = tempDir("readdir-option-values", tree);
expect(summarize(readdirSync(String(dir), options as any))).toEqual(expected);
expect(summarize(await readdirCallback(String(dir), options))).toEqual(expected);
expect(summarize(await promises.readdir(String(dir), options as any))).toEqual(expected);
},
);

// Non-nullish, non-boolean `recursive`: a type error in the sync and callback
// forms (thrown synchronously, before the callback is ever called), plain
// truthiness in the promise form.
const nonBooleanRecursive: [recursive: unknown, expected: typeof flat][] = [
[0, flat],
[1, deep],
["", flat],
["x", deep],
[{}, deep],
[[], deep],
[0n, flat],
];

it.each(nonBooleanRecursive)("readdirSync and readdir reject { recursive: %p }", (recursive, _expected) => {
using dir = tempDir("readdir-option-values", tree);
const invalidArgType = expect.objectContaining({ code: "ERR_INVALID_ARG_TYPE" });
expect(() => readdirSync(String(dir), { recursive } as any)).toThrow(invalidArgType);
const callback = jest.fn();
expect(() => fs.readdir(String(dir), { recursive } as any, callback)).toThrow(invalidArgType);
expect(callback).not.toHaveBeenCalled();
});

it.each(nonBooleanRecursive)(
"promises.readdir treats { recursive: %p } as its truthiness",
async (recursive, expected) => {
using dir = tempDir("readdir-option-values", tree);
expect(summarize(await promises.readdir(String(dir), { recursive } as any))).toEqual(expected);
expect(summarize(await promises.readdir(String(dir), { recursive, withFileTypes: 1 } as any))).toEqual({
...expected,
dirents: true,
});
},
);

it("promises.readdir does not mutate the caller's options", async () => {
using dir = tempDir("readdir-option-values", tree);
const options = { recursive: 1, withFileTypes: 0 };
expect(summarize(await promises.readdir(String(dir), options as any))).toEqual(deep);
expect(options).toEqual({ recursive: 1, withFileTypes: 0 });
});

it("promises.readdir still sees inherited options when it coerces recursive", async () => {
using dir = tempDir("readdir-option-values", tree);
const options = Object.create({ withFileTypes: true });
options.recursive = 1;
expect(summarize(await promises.readdir(String(dir), options))).toEqual(deepDirents);
options.recursive = true;
expect(summarize(await promises.readdir(String(dir), options))).toEqual(deepDirents);
});
});

// The error cleanup path previously called MarkedArrayBuffer.destroy() on
// structs stored by-value inside the entries ArrayList, which passed interior
// ArrayList pointers to the allocator (freeing entries.items.ptr for index 0 and
Expand Down
Loading