Repository navigation
Conversation
size-limit report 📦
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4356442. Configure here.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
The coercion of all buffered responses to streaming feels a bit risky without an escape hatch to work around it, since workerd responses don't have a corollary to Deno's info.completed.
I think this is a good compromise, but would recommend adding the explicit hook option for users that find they need to disable this in some cases.
| /^application\/(x-)?ndjson\b/i.test(contentType) || | ||
| /^application\/stream\+json\b/i.test(contentType) || | ||
| (/^text\/plain\b/i.test(contentType) && !contentLength) | ||
| (/^text\/(plain|html|x-component)\b/i.test(contentType) && !contentLength) |
There was a problem hiding this comment.
So, if I'm reading this right, if you have a handler returns new Response(htmlString), without setting a content-length, then it'll be turned into a streaming response, and get transfer-encoding: chunked.
The runtime knows the body length from the string and content-length only when it sends the response. So res.headers.get('content-length') returns null for a buffered string, an ArrayBuffer, c.html(), and other fully-known response bodies. Before this PR, text/html never matched the regex, so the length check didn't matter. Now any HTML without an explicit header counts as "streaming", whether it streams or not.
At least, we should describe this cost in the PR body and changelog, because it changes the wire output for users who aren't using streamed SSR. Also, maybe we could give users a way out? One option is the hook that the issue asked for (for example isStreamingResponse?: (res: Response) => boolean | undefined, where undefined means "use the default"). Then users with buffered HTML can opt out, and users with custom streamed types can opt in. If detection stays the only mechanism, a workerd benchmark (similar to the Deno one in the next PR in the stack) would show whether the CPU cost matters.
There was a problem hiding this comment.
Also, other streamed content types aren't covered, which could be a possible follow-up. Eg, React Router 7 / Remix single fetch sends .data requests as text/x-script (turbo-stream) and streams deferred promises after the handler returns.
The suggested user hook could be used as a workaround for those cases, as well.
There was a problem hiding this comment.
That is absolutely right. I just removed the html one as it could be dangerous and added only text/x-component as this seems to be a safe default. The additional isStreamingResponse is also good and I added it.
| }; | ||
|
|
||
| // Without the `transformstream_enable_standard_constructor` compatibility flag (default from | ||
| // 2022-11-30, but Hydrogen's mini-oxygen uses 2022-10-31), workerd ignores the transformer, so |
There was a problem hiding this comment.
I just checked, and Hydrogen's mini-oxygen is now using a compat date of 2025-04-01.
Suggestion: say "mini-oxygen before 4.0.0 (and any Worker with a compat date before 2022-11-30)" or just "Workers with a compat date before 2022-11-30 (for example older mini-oxygen releases)".
Ie, the workaround is still worth having, but I'd avoid leaning on a comment that we might later go "oh, that's outdated, let's remove this".
There was a problem hiding this comment.
yeah that is better. I like it
… RSC responses Streamed SSR (`text/html`) and RSC (`text/x-component`) responses were classified as non-streaming, so the root span ended when the handler returned and spans that ended while the body streamed were lost. Both content types are now treated as streaming when the response has no Content-Length, the same rule that `text/plain` already uses. The SDK now creates the streamed response with the original response as the init. This keeps `encodeBody: 'manual'`, so workerd no longer compresses an already compressed body a second time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…TML and RSC responses Without the `transformstream_enable_standard_constructor` compatibility flag (compatibility dates before 2022-11-30, which Hydrogen's mini-oxygen uses), workerd ignores the transformer, so `flush` and `cancel` never ran and the span was never sent. Such Workers now end the span at handler return, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…TML and RSC responses
…TML and RSC responses
4356442 to
ee216da
Compare
…TML and RSC responses
This branch has not been deployed
2 participants
closes #22876
closes JS-3236
Description
This adds an
isStreamingResponseoption, so users can keep thehttp.serverspan open until a streamed response has been sent, for example for streamed SSR pages. Spans created while the body streams then stay inside the request.text/x-component(RSC) responses without aContent-Lengthheader now count as streamed by default. The classifier is shared, so this also applies to Bun and Deno.encodeBody: 'manual'are no longer compressed a second time.TransformStreamtransformer, so the SDK ends the span at handler return there.Detection decisions
HTML is not streamed by default, because a buffered
new Response(html)has noContent-Lengthheader either and would also go through theTransformStream. The alternative is to treat HTML without aContent-Lengthas streamed, which fixes streamed SSR without configuration.