-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
base: main
Are you sure you want to change the base?
Conversation
Fixes FileHandle.readableWebStream in mode "byob" when reading into a view that has a byteOffset>0 into the underlying ArrayBuffer. Fixes: nodejs#58817
There was a problem hiding this 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
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
🚀 New features to boost your workflow:
|
@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! |
There was a problem hiding this 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.
Fixes FileHandle.readableWebStream in mode "byob" when reading into a view that has a byteOffset>0 into the underlying ArrayBuffer.
Fixes: #58817