Skip to content

Conversation

himself65
Copy link
Member

Fixes: #58892

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner
  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 26, 2025
@himself65 himself65 marked this pull request as ready for review July 26, 2025 09:47
Copy link

codecov bot commented Jul 26, 2025

Codecov Report

❌ Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.07%. Comparing base (04c5a18) to head (2bd843b).
⚠️ Report is 266 commits behind head on main.

Files with missing lines Patch % Lines
src/node_file.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59220      +/-   ##
==========================================
+ Coverage   90.04%   90.07%   +0.02%     
==========================================
  Files         648      648              
  Lines      191200   191234      +34     
  Branches    37472    37482      +10     
==========================================
+ Hits       172160   172247      +87     
+ Misses      11665    11606      -59     
- Partials     7375     7381       +6     
Files with missing lines Coverage Δ
lib/fs.js 98.18% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/utils.js 99.39% <100.00%> (+0.02%) ⬆️
src/node_file.cc 75.85% <0.00%> (-0.04%) ⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@himself65
Copy link
Member Author

this might be not a good solution. let me find another way

@aduh95
Copy link
Contributor

aduh95 commented Jul 26, 2025

That's definitely a wrong solution, we need Buffer to support non-unicode paths

@himself65
Copy link
Member Author

yeah, I see path = getValidatedPath(path); is a misleading code, the rest of the logic is pretending path is string but it's actually not. If I change getValidatedPath to support buffer + options.encoding I would guess it will have lot of performance regression

@himself65
Copy link
Member Author

I think I find a good way

@himself65 himself65 changed the title fs: convert to string if input path is buffer fs: fix buffer input with recursive mode on readdir Jul 27, 2025
@himself65 himself65 force-pushed the himself65/2025/07/26/buffer branch from 93e0a32 to 2bd843b Compare July 28, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all versions of readdir don't work in recursive mode when used with a buffer argument

3 participants