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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8a88e2
feat(sendStream): add support for file streaming
GalacticHypernova Jan 20, 2024
0d811b5
chore: apply automated fixes
autofix-ci[bot] Jan 20, 2024
7d100e2
Update response.ts
GalacticHypernova Jan 20, 2024
93c1f98
Update response.ts
GalacticHypernova Jan 27, 2024
94200d6
chore: apply automated fixes
autofix-ci[bot] Jan 27, 2024
90339b1
Update response.ts
GalacticHypernova Jan 27, 2024
dededf4
chore: apply automated fixes
autofix-ci[bot] Jan 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/utils/response.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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..

import type { Socket } from "node:net";
import type { H3Event } from "../event";
import type { HTTPHeaderName } from "../types";
Expand Down Expand Up @@ -220,7 +220,9 @@
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 •
edited

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 •
edited

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?

}

Check warning on line 225 in src/utils/response.ts

View check run for this annotation

Codecov / codecov/patch

src/utils/response.ts#L224-L225 

Added lines #L224 - L225 were not covered by tests
// Directly expose stream for worker environments (unjs/unenv)
(event.node.res as unknown as { _data: BodyInit })._data = stream as BodyInit;

Expand Down