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

toWebRequest starts reading request body eagerly, and we should make it lazy (start reading when a body function is called) #570

Open
nksaraf opened this issue Nov 5, 2023 · 6 comments

Comments

@nksaraf
Copy link

nksaraf commented Nov 5, 2023

Right now toWebRequest calls getRequestWebStream eagerly, which starts reading the request body (to make it into a readable stream). I think we should do this lazily so the user can read the body differently if needed. We can make it lazy a few ways, make the body property into a getter that lazily creates the ReadableStream. And the json(), text() helpers will then rely on that.

h3/src/utils/request.ts

Lines 157 to 168 in a94a4fc

export function toWebRequest(event: H3Event) {
return (
event.web?.request ||
new Request(getRequestURL(event), {
// @ts-ignore Undici option
duplex: "half",
method: event.method,
headers: event.headers,
body: getRequestWebStream(event),
})
);
}

@passionate-bram
Copy link
Contributor

After reading the spec for the Fetch API, I think that the approach of a getter will not work.
See: https://fetch.spec.whatwg.org/#dom-request, relevant are list items 35..42.

There it basically states that init['body'] will get joinked out of the init object and put into the request object if the body is deemed suitable.

@nksaraf
Copy link
Author

nksaraf commented Nov 21, 2023

I feel like it's probably possibly to dynamically define that property as want to achieve compliance with that standard and still be lazy, since this is an adapter layer I think it's worth it to be optimal to the usage patterns that our platforms need

@pi0
Copy link
Member

pi0 commented Dec 7, 2023

Checking impl, toWebRequest>getRequestWebStream only a new ReadableStream for node requests by enqueuing the incoming events.

We might provide a second new option to toWebRequest to disable or customize body mapping (we could alternatively make a proxy Request but I tend to avoid it unless really necessary) but first can you please elaborate better with a reproduction when the initialization makes trouble? 🙏🏼

@nksaraf
Copy link
Author

nksaraf commented Dec 9, 2023

I will create a repro, though generally thinking once user makes a toWebRequest call, are the other body reading functions kinda invalid? Maybe we should throw an error there

@nksaraf
Copy link
Author

nksaraf commented Jan 6, 2024

@pi0 solidjs/solid-start#1165 this is a good reproduction of the issue.

[FROM THE ISSUE]

Current behavior 😯
With the new @solid/start, the /_server request gets stuck and shows (pending) as status in Chrome's Network Inspector when using async code inside the middleware. When not using async code, it works fine.

Expected behavior 🤔
The request resolves

Steps to reproduce 🕹
Steps:

Go to codesandbox.io/p/devbox/solid-start-middleware-for-server-with-promise-session-not-working-6kz3tm?file=%2Fsrc%2Fmiddleware.ts%3A31%2C57
Run the dev server
Click on Generate New button in the preview window, and see the that the code actually executes the console.log but the request hangs as can be seen Network Inspector
If you uncomment the async code inside the middleware.ts, you can see it does work as expected

@pi0
Copy link
Member

pi0 commented Jan 25, 2024

I would appreciate a minimal (h3-only) reproduction in order to follow this up 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants