fs: make FileHandle.readableWebStream always create byte streams
#55461
+47
−95
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
fs
isker
commented
Oct 19, 2024
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
0e81802 to
57bfbb3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55461 +/- ##
==========================================
- Coverage 88.41% 88.41% -0.01%
==========================================
Files 652 654 +2
Lines 186918 187855 +937
Branches 36077 36145 +68
==========================================
+ Hits 165271 166095 +824
- Misses 14901 15000 +99
- Partials 6746 6760 +14
|
jasnell
reviewed
Oct 20, 2024
RedYetiDev
added
the
experimental
RedYetiDev
reviewed
Nov 7, 2024
RedYetiDev
reviewed
Nov 7, 2024
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
57bfbb3 to
88496b4
Compare
RedYetiDev
reviewed
Nov 7, 2024
|
Assuming this lands in 23.X.0, we can remove the warning in 23.(X + 1).0 Although, because this is experimental, a warning isn't technically required, so you have the final say (IMO) whether there is one to add |
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
88496b4 to
d747a6f
Compare
|
Edit: Sorry, it's already marked as such!
|
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
d747a6f to
946019e
Compare
|
@RedYetiDev can you take another look? |
RedYetiDev
reviewed
Nov 17, 2024
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
946019e to
86a1021
Compare
The original implementation of the experimental `FileHandle.readableWebStream` API created non-`type: 'bytes'` streams, which prevented callers from creating `mode: 'byob'` readers from the returned stream, which means they could not achieve the associated "zero-copy" performance characteristics. Then, nodejs#46933 added a parameter allowing callers to pass the `type` parameter down to the ReadableStream constructor, exposing the same semantics to callers of `FileHandle.readableWebStream`. But there is no point to giving callers this choice: FileHandle-derived streams are by their very nature byte streams. We should not require callers to explicitly opt in to having byte stream semantics. Moreover, I do not see a situation in which callers would ever want to have a non-bytes stream: bytes-streams only do anything differently than normal ones if `mode: 'byob'` is passed to `getReader`. So, remove the `options` parameter and always create a ReadableStream with `type: 'bytes'`. Fixes nodejs#54041.
isker
force-pushed
the
filehandle-readablestream-always-bytes
branch
from
86a1021 to
41c448c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original implementation of the experimental
FileHandle.readableWebStreamAPI created non-type: 'bytes'streams, which prevented callers from creatingmode: 'byob'readers from the returned stream, which means they could not achieve the associated "zero-copy" performance characteristics.Then, #46933 added a parameter allowing callers to pass the
typeparameter down to the ReadableStream constructor, exposing the same semantics to callers ofFileHandle.readableWebStream.But there is no point to giving callers this choice: FileHandle-derived streams are by their very nature byte streams. We should not require callers to explicitly opt in to having byte stream semantics. Moreover, I do not see a situation in which callers would ever want to have a non-bytes stream: bytes-streams only do anything differently than normal ones if
mode: 'byob'is passed togetReader.So, remove the
optionsparameter and always create a ReadableStream withtype: 'bytes'.Fixes #54041.