Skip to content

Refactor body handling to use ReadableStream#32

Open
conico974 wants to merge 3 commits into
mainfrom
conico/stream
Open

Refactor body handling to use ReadableStream#32
conico974 wants to merge 3 commits into
mainfrom
conico/stream

Conversation

@conico974

Copy link
Copy Markdown
Contributor

Refactor the handling of request and response bodies to utilize ReadableStream across converters and middleware, improving consistency and performance in body processing.

…ddleware

Co-authored-by: Copilot <copilot@github.com>
@pkg-pr-new

pkg-pr-new Bot commented May 9, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@opennextjs/cloudflare@32
npm i https://pkg.pr.new/@opennextjs/aws@32

commit: bd06278

@conico974 conico974 marked this pull request as ready for review May 9, 2026 10:31
Copilot AI review requested due to automatic review settings May 9, 2026 10:31
devin-ai-integration[bot]

This comment was marked as resolved.

This comment was marked as resolved.

Co-authored-by: Copilot <copilot@github.com>
devin-ai-integration[bot]

This comment was marked as resolved.


const middleware = await middlewareLoader();

const [bodyForMiddleware, bodyForForward] = internalEvent.body?.tee() ?? [undefined, undefined];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i wonder if this is going to be unnecessary for the majority of apps. as in, if someone isn't going to read the body in the middleware, we're probably going ot tee the stream but never make use of that. i wonder if there's some like lazy tee or something we could do potentially.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't know anything about a lazy tee, and to be fair in most case there will be no body.
I guess we could add an option (or an env variable) to not forward the body to the middleware if not needed

Comment on lines +55 to +58
if (!streamDone) {
streamDone = true;
reader.cancel().catch(() => {});
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is it worth waiting for the current read to finish before cancelling, if there's on in-progress? something like toggle streamdone, wait for reading to be false, and in the pump below dont do it if streamdone is true, then cancel here after reading is false? WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess we could do something like that. I don't see any downsides to this at the moment

.then(({ done, value }) => {
if (done) {
streamDone = true;
reader.releaseLock();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hmm shouldnt the release also happen in the catch as well?

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