Skip to content

fs: fix for FileHandle.readableWebStream #58842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pdaehne
Copy link

@pdaehne pdaehne commented Jun 26, 2025

Fixes FileHandle.readableWebStream in mode "byob" when reading into a view that has a byteOffset>0 into the underlying ArrayBuffer.

Fixes: #58817

Fixes FileHandle.readableWebStream in mode "byob" when reading into
a view that has a byteOffset>0 into the underlying ArrayBuffer.

Fixes: nodejs#58817
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jun 26, 2025
@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I think we need to edit the return byteofset field in cpp to solve this problem, I wonder if I am thinking wrong here

Copy link

codecov bot commented Jun 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (100c6da) to head (0421c9e).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58842      +/-   ##
==========================================
- Coverage   90.10%   90.08%   -0.02%     
==========================================
  Files         640      640              
  Lines      188384   188446      +62     
  Branches    36932    36964      +32     
==========================================
+ Hits       169735   169758      +23     
- Misses      11356    11409      +53     
+ Partials     7293     7279      -14     
Files with missing lines Coverage Δ
lib/internal/fs/promises.js 98.09% <100.00%> (ø)

... and 47 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.

@pdaehne
Copy link
Author

pdaehne commented Jun 26, 2025

@mertcanaltin Have a look at this example in the Streams specification, it is more or less a copy of the code in Node.js, and they also use 0 as the "offset" parameter for "FileHandle.read()":

https://streams.spec.whatwg.org/#example-rbs-pull

@mertcanaltin
Copy link
Member

@mertcanaltin Have a look at this example in the Streams specification, it is more or less a copy of the code in Node.js, and they also use 0 as the "offset" parameter for "FileHandle.read()":

thanks!

@LiviaMedeiros LiviaMedeiros changed the title lib: fix for FileHandle.readableWebStream fs: fix for FileHandle.readableWebStream Jun 26, 2025
Copy link
Member

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this!

The correct subsystem (prefix in the commit message) should be fs:, this also can be adjusted by a collaborator who lands it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in fileHandle.readableWebStream implementation
6 participants