feat: optimize readdir performance and align API with Node.js #1
Conversation
WalkthroughThis change refactors the directory-reading API to support both recursive and non-recursive listing modes, with optional Dirent objects instead of only strings. The TypeScript types, Rust implementation, and tests are updated accordingly, with field name changes and expanded option support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.d.ts (1)
9-9: Fix the return type forreaddir.The async
readdirfunction returnsPromise<unknown>, which loses type safety. It should return the same type asreaddirSync:Promise<Array<string> | Array<Dirent>>.Apply this diff:
-export declare function readdir(path: string, options?: ReaddirOptions | undefined | null): Promise<unknown> +export declare function readdir(path: string, options?: ReaddirOptions | undefined | null): Promise<Array<string> | Array<Dirent>>
🧹 Nitpick comments (1)
src/readdir.rs (1)
130-148: Consider logging fallback path.The fallback on line 143 silently handles
strip_prefixfailures by returning just the filename. While this prevents crashes, it could make debugging harder if the fallback is unexpectedly triggered. Consider logging or documenting when this case occurs.Example enhancement:
match p.strip_prefix(root) { Ok(relative) => relative.to_string_lossy().to_string(), - Err(_) => e.file_name().to_string_lossy().to_string(), // Fallback + Err(_) => { + // Fallback: this shouldn't normally happen + eprintln!("Warning: Failed to strip prefix for path: {:?}", p); + e.file_name().to_string_lossy().to_string() + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__test__/readdir.spec.ts(2 hunks)benchmark/readdir.ts(1 hunks)index.d.ts(2 hunks)src/lib.rs(1 hunks)src/read_dir.rs(0 hunks)src/readdir.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/read_dir.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/lib.rs (1)
src/readdir.rs (1)
readdir(179-181)
__test__/readdir.spec.ts (1)
src/readdir.rs (1)
readdir(179-181)
src/readdir.rs (1)
index.d.ts (2)
ReaddirOptions(11-16)Dirent(3-7)
🔇 Additional comments (16)
src/readdir.rs (5)
18-33: LGTM!The struct definitions are well-designed. Optional fields provide flexibility for default values, and the Clone derivation is necessary for the async Task implementation.
36-58: LGTM!The options handling is sound. Empty path defaults to ".", existence check provides proper ENOENT error, and default values align with expected behavior.
151-181: LGTM!The async implementation follows the standard napi-rs pattern. The synchronous wrapper and Task implementation are correctly structured. Cloning in
compute()is necessary for the mutable borrow context.
59-98: Performance optimization correctly implemented.The non-recursive path efficiently uses
std::fs::read_dirinstead ofjwalk. The mutually exclusive Vec allocation pattern is correct and avoids unnecessary allocations. Thefile_type()call includes a safe fallback withunwrap_or(false).The
parent_pathbehavior matches Node.js semantics—it is set to the directory path passed to the function, so relative paths like "." and absolute paths are preserved consistently across all entries.
100-129: RayonNewPool(0) correctly uses automatic thread pool sizing.Line 103–104: The code correctly uses
Parallelism::RayonNewPool(0)whenconcurrencyisNone. According to jwalk's behavior, passing 0 to RayonNewPool instructs Rayon to automatically choose the thread pool size (respectingRAYON_NUM_THREADSif set, otherwise defaulting to the number of logical CPUs). This is the intended behavior and not a bug.src/lib.rs (1)
13-16: LGTM!The module rename from
read_dirtoreaddircorrectly aligns with the new file structure.benchmark/readdir.ts (1)
12-39: Excellent benchmark coverage!The expanded benchmarks comprehensively test the new API surface: default behavior,
withFileTypes,recursive, and various concurrency combinations. This provides valuable performance comparison data against Node.js.index.d.ts (3)
3-7: LGTM!The
pathtoparentPathrename improves clarity, making it explicit that this field represents the parent directory path, not the full file path.
11-16: LGTM!The added
recursiveandwithFileTypesoptions correctly align with the Rust implementation and Node.js API.
18-21: LGTM!The return type correctly reflects the Either behavior: returning string arrays by default and Dirent arrays when
withFileTypes: true.__test__/readdir.spec.ts (6)
4-15: LGTM!The test correctly verifies the default behavior returns strings. The type check on line 11 and the string equality check on line 13 properly validate the return type.
17-44: LGTM!The test comprehensively validates Dirent objects when
withFileTypes: true. The type guards (lines 26-31, 36-38, 41-43) properly handle the union type, and the checks fornameandisDirproperties verify the Dirent structure.
46-51: LGTM!The async test correctly validates that
readdirreturns strings by default and includes expected files.
53-67: Good test coverage for concurrency.The tests correctly verify that concurrency works with recursive mode. The comment on line 56 helpfully explains that concurrency requires recursive mode.
69-84: LGTM!The
skipHiddentest correctly handles both string and Dirent return types using the ternary operator on lines 75 and 79. The test logic properly validates hidden file filtering.
86-89: LGTM!Error handling tests correctly verify that both sync and async variants throw on non-existent directories.
| // advanced usage | ||
| // readdirSync('./src', { | ||
| // recursive: true, | ||
| // concurrency: 8, | ||
| // ignore: ['.git'], | ||
| // }); |
There was a problem hiding this comment.
Update or remove outdated comment.
The example comment references an ignore option that is not implemented in ReaddirOptions. This could mislead developers.
Apply this diff to remove the outdated reference:
// advanced usage
// readdirSync('./src', {
// recursive: true,
// concurrency: 8,
-// ignore: ['.git'],
// });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // advanced usage | |
| // readdirSync('./src', { | |
| // recursive: true, | |
| // concurrency: 8, | |
| // ignore: ['.git'], | |
| // }); | |
| // advanced usage | |
| // readdirSync('./src', { | |
| // recursive: true, | |
| // concurrency: 8, | |
| // }); |
🤖 Prompt for AI Agents
In src/readdir.rs around lines 11 to 16, the commented example shows an `ignore`
option that no longer exists on ReaddirOptions; remove the `ignore: ['.git'],`
fragment from the example (or update it to a valid option if you prefer) so the
comment reflects the current API and cannot mislead developers.
|
I plan to add a JS wrapper layer in a future PR to optimize the speed of scanning flat directory trees |
Description
This PR optimizes the performance of
readdirandreaddirSyncby usingstd::fsfor simple, non-recursive cases, and aligns the API behavior with Node.js by introducing thewithFileTypesoption and returning an array of strings by default.Changes
Performance Optimization:
readdirnow usesstd::fs::read_dirdirectly instead ofjwalk. This significantly reduces overhead for common use cases.withFileTypesis explicitly set totrue.API Alignment:
withFileTypes: booleanoption.readdirnow returnsstring[](file names) by default, matching Node.jsfs.readdirbehavior. Previously it returnedDirent[].withFileTypes: trueis passed, it returnsDirent[]containingname,parentPath, andisDir.Refactoring:
lsfunction insrc/readdir.rsto handle both return types (Vec<String>andVec<Dirent>) usingEither.Benchmarks
Benchmarks run on
node_modulesdirectory:Checklist
cargo check,npm run lint)__test__/readDir.spec.tsbenchmark/readdir.tsSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.