-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Body parsing #50
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
Comments
I agree that there probably is some behavior that is possible in all reasonable target environments that would work the vast majority of the time. Would it be worth it to provide this common API to each session getter / endpoint / whatever, and then for each to also potentially be passed lower-level environment-specific objects? We could encourage everyone to use the common APIs, but leave the others in as an escape hatch. As I write this, I realize that, while this would help in situations where we need more control over things, it would not help in situations where we specifically do not want to parse the whole body for performance reasons. If it's already been parsed, it's already been parsed - and we wouldn't even be able to stream anything from it, because the stream would have been closed. So perhaps I take back a lot of this comment. |
If you could export e.g. if (`${method}Raw` in mod) {
mod[`${methodRaw}`](req, res);
} else if (method in mod) {
const result = await mod[method]({
path: get_path: req.url,
body: await read_body_from_stream(req),
// ...
};
res.writeHead(result.status || 200, result.headers || {});
res.end(result.body);
} else {
return { status: 501 };
} ...though I'm not suggesting we actually do that |
There's a My hesitation would just be that you may want a programmatic interface for on-demand parsing (like in last snippet) so that you can reuse/share same approach across adapters instead of having a middleware in some places & programmatic callers in others. |
My current thinking (and what I'm working on right now) is that app-utils should have a helper that lets you do const body = await get_body(req); which would be used by the dev server and adapter-node. Lambda-based adapters would have a helper for processing |
Yup, sounds like the right approach. Have been doing the same thing w/ freshie but custom implementations for each platform-type. No point trying to make 1 universal utility to handle them all IMO. Was just offering the |
Since endpoint handlers don't get the
req
object, but rather a normalized{ host, path, params, query, body }
object (still not sure abouthost
, and I guess it should probably also getheaders
), adapters (and the dev server) will need to parse the body before callingrender
.In Lambda environments...
...we presumably need to
JSON.parse(event.body)
, except when it's base64 encoded in which case we need to do... something else? No idea what happens to form data etc. But after you've jumped through whatever hoops, the body is available.In Vercel,
req.body
is already populated according to the following rules:In adapter-node and the dev server, we need to figure this out ourselves, possibly using body-parser and something like multer?
I think the goal should be for the behaviour to be as similar as possible between environments. Vercel's approach seems like something we could adopt for adapter-node and the dev server. Are there any bases that aren't covered by that approach?
In #46 (comment) @Conduitry said
which is fair. My instinct though is that it is worth normalizing, and the fact that Lambda and Vercel both pass an already-constructed body to handlers is strong evidence that streamable bodies are more of an academic benefit than a real one.
The text was updated successfully, but these errors were encountered: