fix: support string encoding parameter in readFile#17
Conversation
- Add Either<String, ReadFileOptions> to accept both string and object forms - Add normalize_read_file_options helper for parameter normalization - Add tests for string encoding, object encoding, and no encoding cases - Fixes issue #16 where readFile(path, "utf-8") incorrectly returned Buffer
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR refactors readFile option handling to accept either string encoding or full options objects via an Either type, introduces a normalization helper function, and adds tests verifying correct return types (string with encoding, Buffer without). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (1)
src/read_file.rs (1)
155-161: Consider adding test coverage forreadFileSyncwith string encoding.The sync function also supports the new string encoding parameter, but there's no test verifying
readFileSync('./package.json', 'utf-8')returns a string. The existing tests only use the object form.💡 Suggested test to add in __test__/read_file.spec.ts
test('readFileSync: should read file as string with encoding as string param', (t) => { const result = readFileSync('./package.json', 'utf-8') t.is(typeof result, 'string') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/read_file.rs` around lines 155 - 161, Add a unit test to cover the string-encoding variant of the synchronous API: create a test in __test__/read_file.spec.ts that calls readFileSync('./package.json', 'utf-8') and asserts the result is a string (e.g. typeof result === 'string'); this ensures read_file_sync (which forwards to read_file_impl) correctly returns a string when a plain string encoding is passed instead of the object form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/read_file.rs`:
- Around line 155-161: Add a unit test to cover the string-encoding variant of
the synchronous API: create a test in __test__/read_file.spec.ts that calls
readFileSync('./package.json', 'utf-8') and asserts the result is a string (e.g.
typeof result === 'string'); this ensures read_file_sync (which forwards to
read_file_impl) correctly returns a string when a plain string encoding is
passed instead of the object form.
1 participant
🌌 Fix for Issue #16
Problem
readFile(path, "utf-8")was ignoring the string encoding parameter and always returningBuffer, inconsistent with Node.js nativefs.readFile()behavior.Root Cause
The Rust function signature only accepted
Option<ReadFileOptions>(object form), so when a string was passed:readFile("file.txt", "utf-8")→options.encodingwasundefined→ returnedBuffer❌readFile("file.txt", { encoding: "utf-8" })→ worked correctly ✅Solution
Use NAPI-RS
Either<String, ReadFileOptions>to accept both parameter forms:Changes
readFile(path, "utf-8")→ returns String ✅readFile(path, { encoding: "utf-8" })→ returns String ✅readFile(path)→ returns Buffer ✅Testing
All new tests pass. The API is now fully consistent with Node.js native
fs.readFile().Closes #16
Summary by CodeRabbit
Tests
New Features