Skip to content

Conversation

@Rich-Harris
Copy link
Member

This bypasses the buffering issues encountered in #591

@benmccann
Copy link
Member

benmccann commented Mar 24, 2021

@stalkerg could you take a look at this?

@benmccann
Copy link
Member

benmccann commented Mar 24, 2021

Ah, so if I'm understanding this correctly, if the user calls await response.text() then that call is now made only a single time and we're not making our own call? If so, then I think that will work for the common case of getting the whole response body

I do not think that this approach will be compatible with streaming however. Are we comfortable with shutting off approaches that will let us do streaming in the future? Or should we also try to solve for the streaming use case now while we're working on this?

@Rich-Harris
Copy link
Member Author

Correct, text() only gets called once. (Calling it a second time would be an error regardless of the proxy.)

I do not think that this approach will be compatible with streaming however

Why? All it means is that the streamed response doesn't get inlined. Think of the inlining as a progressive enhancement

@benmccann benmccann added the bug Something isn't working label Mar 24, 2021
@benmccann
Copy link
Member

I guess you can skip the serialized data with the way hydration works today. I was thinking you wouldn't be able to do that longer-term. E.g. if we have a mode where you trust the contents of the page instead of reconciling every element that might not work. But maybe that's too far away or too speculative to worry about right now

@stalkerg
Copy link
Contributor

@benmccann yes, this fix is working for me, thanks!

@Rich-Harris
Copy link
Member Author

excellent, will get this merged and released. thanks for verifying

@Rich-Harris Rich-Harris merged commit 531cf0c into master Mar 25, 2021
@Rich-Harris Rich-Harris deleted the gh-591-2 branch March 25, 2021 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants