Skip to content
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

feat(streams/unstable): toByteStream() #6046

Merged
merged 11 commits into from
Oct 8, 2024

Conversation

BlackAsLight
Copy link
Contributor

@BlackAsLight BlackAsLight commented Sep 25, 2024

This pull request introduces a small function to @std/streams that allows someone to easily upgrade a ReadableStream<Uint8Array> to support BYOB if it doesn't already.

Example

import { upgradeReadable } from "@std/streams/unstable-upgrade-readable-stream";

const reader = upgradeReadable(ReadableStream.from([
  new Uint8Array(100),
  new Uint8Array(78),
  new Uint8Array(125),
]))
  .getReader({ mode: "byob" });

reader.releaseLock()

@kt3k
Copy link
Member

kt3k commented Sep 25, 2024

This might be useful in some cases, but I'm not sure if it's common to call this operation/util 'upgrade'...('upgrade' reminds me of upgrading http connection to websocket connection). How about calling this 'toReadableByteStream'?

@crowlKats @lucacasonato What do you think about this?

@BlackAsLight
Copy link
Contributor Author

toByteStream is a slightly shorter name.

I use a similar function a lot in the cbor pull request and thought it's unique enough and useful enough to offer as a utility.

I also found if you don't end up enqueuing or responding in each pull then you end up with that promise left in the event loop error.

@kt3k
Copy link
Member

kt3k commented Sep 25, 2024

toByteStream sounds good to me

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.76%. Comparing base (ea85486) to head (2fd5a88).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6046      +/-   ##
==========================================
+ Coverage   96.75%   96.76%   +0.01%     
==========================================
  Files         508      509       +1     
  Lines       39140    39185      +45     
  Branches     5793     5803      +10     
==========================================
+ Hits        37869    37917      +48     
+ Misses       1229     1226       -3     
  Partials       42       42              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BlackAsLight BlackAsLight changed the title feat(streams/unstable): upgradeReadable() feat(streams/unstable): toByteStream() Sep 25, 2024
@BlackAsLight
Copy link
Contributor Author

I'd just like to say that getting this function right was quite hard as there always seemed like another hiccup.

I've found that when using the min property, value will always be a Uint8Array, even when done is true and TypeScript infers it might be undefined. value is still a Uint8Array, even if that length is zero. value when done is true will contain the amount it was able to get that's less than min.

Copy link
Member

@kt3k kt3k 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!

@kt3k kt3k merged commit 5a1da8d into denoland:main Oct 8, 2024
16 checks passed
@BlackAsLight BlackAsLight deleted the upgrade_readable branch October 11, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants