sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.43k stars 1.89k forks source link

Body parsing #50

Closed Rich-Harris closed 3 years ago

Rich-Harris commented 3 years ago

Since endpoint handlers don't get the req object, but rather a normalized { host, path, params, query, body } object (still not sure about host, and I guess it should probably also get headers), adapters (and the dev server) will need to parse the body before calling render.

In Lambda environments...

exports.handler = async function(event) {
  // event.body is a 'A JSON string of the request payload.'
  // event.isBase64Encoded is "A boolean flag to indicate if the applicable request payload is Base64-encode"
}

...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:

Content-Type header Value of req.body
No header undefined
application/json An object representing the parsed JSON sent by the request.
application/x-www-form-urlencoded An object representing the parsed data sent by with the request.
text/plain A string containing the text sent by the request.
application/octet-stream A Buffer containing the data sent by the request.

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 https://github.com/sveltejs/kit/issues/46#issuecomment-713837405 @Conduitry said

I don't currently have a situation in mind where having access to the HTTP headers (for session) would be insufficient, or where having access to HTTP headers and a readable stream (for endpoints) would be insufficient - I'm concerned about a situation where someone only ever intends to build a Node server and really just wants to be able to access req and res directly, and can't

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.

Conduitry commented 3 years ago

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.

Rich-Harris commented 3 years ago

If you could export e.g. getRaw alongisde get, then you could skip reading/parsing the stream...

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

lukeed commented 3 years ago

There's a @polka/parse for Node.js adapter(s). You can mount the various methods individually to handle all the different Content-Type values. And then you'd have to use raw for file uploads.

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.

Rich-Harris commented 3 years ago

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 event.body, in adapter-vercel you'd just pass along req.body, etc.

lukeed commented 3 years ago

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 parse package as a quick starting point, in case you didn't already have one. But of course you do :D