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

Handling streaming request/response bodies #3419

Closed
Rich-Harris opened this issue Jan 19, 2022 · 12 comments · Fixed by #5291
Closed

Handling streaming request/response bodies #3419

Rich-Harris opened this issue Jan 19, 2022 · 12 comments · Fixed by #5291
Labels
feature request New feature or request
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

Prior discussions: #70, #1563

As of #3384, hooks and endpoints receive a standard Request object, which among other things makes it possible to POST files:

// handles submission from a <form enctype="multipart/form-data">
export async function post({ request }) {
  const body = await request.formData();
  const file = await body.get('myfile');
  // ...
}

The file is buffered in memory, however, which means there's a practical limit on how large it can be.

Similarly, there's no good way to read a stream of binary data, or respond with a stream.

Describe the proposed solution

We need to solve this at a high level and at a low level. At the high level, it would be good to have utility functions for dealing with multipart form data specifically — something like this, perhaps:

import { multipart } from '$app/somewhere';

export async function post({ request }) {
  for await (const part of multipart(request)) {
    if (part.filename) {
      const uploader = create_s3_uploader(BUCKET, part.filename);
      for (const chunk of part) {
        uploader.write(chunk);
      }
    }
  }

  return { status: 201 };
}

For octet streams it might look like this:

import { stream } from '$app/somewhere';

export async function post({ request }) {
  for await (const chunk of stream(request)) {
    // ...
  }

  return { status: 201 };
}

At the low level, it should be possible to create your own stream reader and return your own ReadableStream bodies:

export async function post({ request }) {
  const reader = request.body.getReader();

  let chunk;
  while (chunk = await reader.read()) {
    if (chunk.done) break;
    await do_something_with(chunk.value);
  }

  return {
    body: new ReadableStream({
      start(controller) {
        // ...
      },
      cancel() {
        // ...
      }
    })
  };
}

Unfortunately there's a wrinkle: ReadableStream isn't available in older versions of Node, and the Request and Response objects (as polyfilled by node-fetch) use node streams instead of ReadableStream. It may therefore be necessary to polyfill ReadableStream during development (and in adapter-node, and lambdas) and convert between that and node streams. (On the request object, this would probably mean creating a Proxy that replaces body with a ReadableStream with the same contents as the node stream. Hopefully this is actually possible.)

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@Rich-Harris
Copy link
Member Author

We may be able to make life easier in future by swapping out node-fetch for undici.fetch, which uses ReadableStream: https://undici.nodejs.org/#/?id=undicifetchinput-init-promise. Unfortunately it's not production-ready yet

@benmccann
Copy link
Member

Looks like there's a PR for adding stream support to node-fetch: node-fetch/node-fetch#1481

@benmccann benmccann added this to the 1.0 milestone Jan 27, 2022
@kaktus40
Copy link

Hello,
I could stream uploading data with @sveltejs/kit version 1.0.0-next.245 from a form data.:
uploader.ts is

import { readFileSync, createWriteStream } from 'fs';
import { tmpdir } from 'os';
import { join } from 'path';
import busboy from 'busboy';
import { pipeline } from 'stream/promises';

import type { RequestHandler } from '@sveltejs/kit';

export const post: RequestHandler = async ({ request }) => {
	const content = request.headers.get('content-type');

	const saveTo = join(tmpdir(), 'testFile');
	const bb = busboy({
		headers: {
			'content-type': content
		}
	});
	bb.on('file', (name, file, info) => {
		const { filename, encoding, mimeType } = info;
		console.log(
			`File [${name}]: filename: %j, encoding: %j, mimeType: %j`,
			filename,
			encoding,
			mimeType
		);
		file.pipe(createWriteStream(saveTo));
	});
	bb.on('field', (name, val, info) => {
		console.log(`Field [${name}]: value: %j`, val);
	});
	bb.on('close', () => {
		console.log('Done parsing form!');
	});

	await pipeline(request.body as any, bb);

	 return {
	 	status: 302,
	 	headers: {
	 		location: '/uploader'
	 	}
	 };
};

uploader.svelte is

<form method="POST" enctype="multipart/form-data" action="/authentification/uploader">
	<input type="file" name="filefield" /><br />
	<input type="text" name="textfield" /><br />
	<input type="submit" />
</form>

@pixelmund
Copy link
Contributor

We may be able to make life easier in future by swapping out node-fetch for undici.fetch, which uses ReadableStream: https://undici.nodejs.org/#/?id=undicifetchinput-init-promise. Unfortunately it's not production-ready yet

nodejs/node#41749

@benmccann
Copy link
Member

We may be able to make life easier in future by swapping out node-fetch for undici.fetch, which uses ReadableStream: https://undici.nodejs.org/#/?id=undicifetchinput-init-promise. Unfortunately it's not production-ready yet

This seems like a good idea to me. The main issue I see is that it's only supported on Node 16.5+ and AWS Lambda doesn't yet support Node 16 so neither does Netlify or anything else built on top of Lambda.

@ardatan
Copy link

ardatan commented Apr 7, 2022

We use cross-undici-fetch that brings Fetch API in a better way and it also respects and ponyfills stuff according to the node version. We use it in GraphQL Yoga that uses Fetch API to handle HTTP request pipeline in platform agnostic way. As a maintainer of those two, I'd love to help you if you want to use it :)

GraphQL Yoga uses Request.formData to parse multipart requests(file uploads etc) and ReadableStream to handle incremental delivery(SSE or multipart). cross-undici-fetch helps us to make this functionality work in platform agnostic way in a single line of code;
https://github.com/dotansimha/graphql-yoga/blob/master/packages/common/src/getGraphQLParameters.ts#L50
https://github.com/dotansimha/graphql-yoga/blob/master/packages/common/src/getResponse.ts#L104

So as GraphQL Yoga maintainers, we can support different platforms besides NodeJS like CF Workers, Deno etc.

@Rich-Harris Rich-Harris changed the title Handling large request/response bodies Handling streaming request/response bodies May 5, 2022
@Rich-Harris Rich-Harris mentioned this issue May 15, 2022
11 tasks
@chientrm
Copy link
Contributor

Is this feature already available in svelte-kit or waiting for the next release? 🤔

@sambonbonne
Copy link

@chientrm it seems not, #5113 need to be completed and closed before handling this issue. The PR is (was?) waiting for a fix from undici.

@chientrm
Copy link
Contributor

chientrm commented Jun 20, 2022

@chientrm it seems not, #5113 need to be completed and closed before handling this issue. The PR is (was?) waiting for a fix from undici.

That's a lot of work by replacing nodefetch with undici. Should we only use undici for body streaming and leave the rest with nodefetch.

@sambonbonne
Copy link

Should we only use undici for body streaming and leave the rest with nodefetch.

I'm not involved with the Svelte project so my answer will only reflects my own opinion.

I think having to dependencies to do something that similar would only add maintenance (because you have to dependencies to maintain), increase attack surface (two dependencies for the same thing means two security checks to do when you could have only one), create some conditional code where you can use one dependency instead of two with an if (and confuse users and/or maintainers) and grow all node_modules/ for nothing.

IDK why the undici switch is needed (or just wanted) by Svelte Kit maintainers but I understand that having two dependencies for the same thing can be a problem. I want this issue to be resolved but I guess the priority is to switch and I think if we are patient, we'll be really happy when the issue is resolved properly!

@0gust1
Copy link
Contributor

0gust1 commented Jul 5, 2022

🎉 🎉 🎉 🎉 🎉 🙏

@xpluscal
Copy link

I hope this is relevant here.
I am importing/using an npm package on a server endpoint and getting the errors
response.body.getReader is not a function.

From the conversations I see that this might be related here - how would I with the new feature enable the package to use ReadableStream?

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

Successfully merging a pull request may close this issue.

10 participants