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: Use ReadableStream instead of a stream of bytes #23

Merged
merged 6 commits into from
Dec 31, 2024

Conversation

dariusc93
Copy link
Contributor

Previously, we would use an AsyncIterator for handling rust streams, and in some situations they do work well but it can be a problem when it comes to handling streams for files as these would be chunks of data that is streamed. While an AsyncIterator could handle it, it may not be idiomatic to work with other functions without redundant workarounds. Additionally, in some cases, would require to buffer the data, which is not ideal for large sums of data.

This PR, while more of a PoC, would begin the process of using ReadableStream in place of AsyncIterator, which should work best for different situations such as creating a blob to download files without the need to buffer them. In other situations, could probably be used in place of AsyncIterator for events, however that would need discussions to determine if that might be suitable.

@dariusc93 dariusc93 self-assigned this Dec 22, 2024
@dariusc93 dariusc93 marked this pull request as ready for review December 30, 2024 15:14
// display
append_img_uri(uri);

// dowbloadable link
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@ashneverdawn ashneverdawn left a comment

Choose a reason for hiding this comment

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

lgtm

@ashneverdawn
Copy link
Contributor

note, this will require changes in UplinkWeb similar to the constellation example.

@dariusc93 dariusc93 merged commit f12b0a4 into main Dec 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants