sveltejs / kit

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

Handling streaming request/response bodies #3419

Closed Rich-Harris closed 2 years ago

Rich-Harris commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Looks like there's a PR for adding stream support to node-fetch: https://github.com/node-fetch/node-fetch/pull/1481

kaktus40 commented 2 years ago

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 commented 2 years ago

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

https://github.com/nodejs/node/pull/41749

benmccann commented 2 years ago

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 commented 2 years ago

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.

chientrm commented 2 years ago

Is this feature already available in svelte-kit or waiting for the next release? πŸ€”

sambonbonne commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

πŸŽ‰ πŸŽ‰ πŸŽ‰ πŸŽ‰ πŸŽ‰ πŸ™

xpluscal commented 2 years ago

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?