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

fix(sendStream): add support for file streaming #624

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Jan 20, 2024

πŸ”— Linked issue

Closes #623

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR adds support for file streaming when used with unstorage by converting the buffer to a readable.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@GalacticHypernova GalacticHypernova changed the title feat(sendStream): add support for file streaming fix(sendStream): add support for file streaming Jan 20, 2024
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8345c1f) 77.80% compared to head (dededf4) 77.78%.
Report is 6 commits behind head on main.

Files Patch % Lines
src/utils/response.ts 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #624      +/-   ##
==========================================
- Coverage   77.80%   77.78%   -0.03%     
==========================================
  Files          47       47              
  Lines        4281     4289       +8     
  Branches      610      612       +2     
==========================================
+ Hits         3331     3336       +5     
- Misses        933      936       +3     
  Partials       17       17              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@@ -1,5 +1,5 @@
import type { OutgoingMessage } from "node:http";
import type { Readable } from "node:stream";
import { Readable } from "node:stream";
Copy link
Member

Choose a reason for hiding this comment

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

We cannot depend on Node.js imports in h3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that's gonna make it trickier then, because the current stream can't handle buffers..

@@ -220,7 +220,9 @@ export function sendStream(
if (!stream || typeof stream !== "object") {
throw new Error("[h3] Invalid stream provided.");
}

if (Buffer.isBuffer(stream)) {
stream = Readable.from([stream]);
Copy link
Member

@pi0 pi0 Jan 24, 2024

Choose a reason for hiding this comment

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

I am wondering since the data source is already in memory as Buffer, would there be any benefits of this over simply using res.end(buff) and let Node.js/OS TCP stack handle streaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a guarantee it would send it as a stream?

Copy link
Member

@pi0 pi0 Jan 25, 2024

Choose a reason for hiding this comment

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

It is a TCP stream always. How to be chunked? it depends on many factors including stack and reverse proxy's behavior and the client receives on the other side. But I guess when we have all the data already in a buffer (memory), passing it to the lower level engine makes the right sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi0 like this?

@GalacticHypernova GalacticHypernova marked this pull request as draft February 1, 2024 16:26
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.

Incompatible streams with raw values from unstorage when used in sendStream
2 participants