sveltejs / kit

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

Ability to stream data (for non-serverless adapters like adapter-node) #1563

Closed jesseskinner closed 2 years ago

jesseskinner commented 3 years ago

Is your feature request related to a problem? Please describe. I have APIs built with Sapper that I'd like to migrate to SvelteKit, but some endpoints rely heavily on using Node streams to pipe data to the response, as there can be gigabytes of data that are too large to serialize to a string server-side in memory. Some of this data is coming from a database, other data is coming from streaming and processing large files that live on AWS S3.

Describe the solution you'd like I'd like to be able to return an async iterator in the body attribute of the response. I realise streaming isn't possible with most (all?) serverless adapters or adapter-static, but would certainly be possible with adapter-node and possibly other adapters that don't yet exist.

Describe alternatives you've considered adapter-node and the development server could expose the Node res object to the request so I could pipe directly to the res object inside the request handler. Otherwise, I will have to split off the API into a standalone Node server that does not live within SvelteKit, or continue to use Sapper and have a much slower build process during development.

How important is this feature to you? I would love to keep my API in SvelteKit as I currently do in Sapper. It's really nice having the API endpoints live in the same router. To split it off into a separate server will incur extra complexity, such as dealing with sharing authentication, keeping the servers in sync, multiple code bases, etc. I would really rather not stay with Sapper long term.

Additional context Add any other context or screenshots about the feature request here.

EDITED: changed the preferred solution from stream object to async iterator, because Node Streams in v10+ are also async iterators.

babichjacob commented 3 years ago

Heavily related to https://github.com/sveltejs/kit/issues/1491 and https://github.com/sveltejs/kit/issues/887.

jesseskinner commented 3 years ago

If we wanted to ensure that returning a stream in the body remains compatible with all adapters, so that this isn't a special adapter-node-only feature, it is possible to collect the contents of a stream in a string for compatibility with adapters that will never support streaming. See stream-to-string for one implementation of this. (Obviously this would have memory limitations but would work with serverless adapters otherwise.)

babichjacob commented 3 years ago

I've thought about this problem before, and my first idea for the API design for this in endpoints is something along these lines:

export async function* get() {
    yield {
      status: 200,
      headers: {
        "Connection": "keep-alive",
        "Content-Type": "multipart/mixed",
        "Transfer-Encoding": "chunked",
      },
    };

    // ... every time you get new data to send (so suppose this is in a for/while loop or waiting for another promise)
    yield {
      headers: {
        "Content-Type": "application/json; charset=utf-8",
      },
      body: newData,
    };
    // return from the function when done
}

// this was modeled after https://github.com/contrawork/graphql-helix/#basic-usage

(Disclaimer: I honestly haven't done streamed responses in Express myself before so I might have missed something about how they work). The original sample has some work with chunk boundaries and Content-Length, but I think it's reasonably simple for SvelteKit to have abstracted this away like above.

This was just an initial thought for someone else to refine / reject; hope it's constructive.

jesseskinner commented 3 years ago

@babichjacob using an asynchronous generator for the body instead of a Node Stream is a great idea - though it wouldn't send new headers on every iteration, so it would just be for the body contents. I like the idea of leaning on JavaScript and away from Node, so that this would be more universal and compatible with Deno as well (and other non-Node JavaScript runtimes that may arise in the future).

UPDATE: it turns out that Node Streams (Node v10+) are already asynchronous iterators! So maybe allowing for an async iterator to be passed to the body would be the sweet spot - allowing existing Node Stream libraries to be used here without jumping through extra hoops.

So maybe something like this would work?

export async function get() {
    return {
      status: 200,
      headers: {
        "Content-Type": "text/csv; charset=utf-8",
      },
      body: generateCSV() // could also be a Node Stream
    };
}

async function* generateCSV() {
   yield "first,row,of,a,csv,file\n";
   yield "second,row,of,a,csv,file\n";
}
jesseskinner commented 3 years ago

Converting an asynchronous iterator to a string (for serverless & static adapters) would be as simple as this:

async function iteratorToString(iterator) {
   let string = '';
   for await (let chunk of iterator) {
      string += String(chunk);
   }
   return string;
}

Though we may also need to deal separately with binary data (Uint8Array) generators, but I'm not sure about it.

jesseskinner commented 3 years ago

@benmccann that's great! I'd be very happy to help with the implementation of this - I'll try to put together a pull request in the next week or so.

babichjacob commented 3 years ago

that's great! I'd be very happy to help with the implementation of this - I'll try to put together a pull request in the next week or so.

Don't forget to wait until multiple team members have commented to figure out what the API should look like—and other things related to that.

kjmph commented 3 years ago

Ah, I had searched for an issue (which didn't exist) and went ahead with an experimental implementation before opening what is now a duplicate issue. Happy to help here if I can. I concur with @jesseskinner's design sensibilities.

MatthiasGrandl commented 3 years ago

Literally the only missing puzzle piece for me. If there is anything I can do to help, ping me.

benmccann commented 3 years ago

I'm pretty comfortable with JavaScript, but it isn't my primary language, so I haven't used async iterators before and don't have any opinion on whether or not it's the best design choice. If you'd like to send an implementation, please feel free as long as you don't mind getting some feedback about reworking it. If you'd rather have that feedback up front I can try to ask some of the other maintainers about this ticket

jesseskinner commented 3 years ago

@benmccann feedback up front would be ideal, but I'll probably end up starting an implementation sometime soon even if I don't hear back. Thanks!

kjmph commented 3 years ago

There is an implementation linked in the issue I created. See here:

https://github.com/sveltejs/kit/compare/master...kjmph:feature/event-source?expand=1

I didn't open a pull request because it suggests opening an issue prior to a PR.

We can expand this beyond just an text/event-stream content-type.

benmccann commented 3 years ago

It would be fine to open a PR referencing this issue, but I do think that you should try to make it more generic than text/event-stream first

juanmait commented 3 years ago

I would vote for having some way of accessing the raw request object on whatever the platform you are. That would allow not only to use streaming on other environments apart from NodeJS but also allow any other use case that requires access to this objects or even any future development or features that will be available on every particular platform. It would mean that the sveltekit team will not have to think too much when new requests to support new things comes in as having access to this objects means that there will always be ways to workaround any problem until a more "user friendly" solution is develop.

In my particular case I want to try streaming on cloudflare workers, but if that doesn't work I might also try vercel. I also need to pipe the requests through a number of transform streams before using the final result. The goal would be to bypass the part in which the rawBody is filled as it's the kind of thing you want if you're using streaming. In any case I will have to use some proxy to send this requests outside sveltekit or may be use CORS. So there are some options but It's a little bit of an inconvenience.

kjmph commented 3 years ago

Hello @benmccann; @jesseskinner and I synced on what a more generic solution would look like. We updated the experimental branch to be generic. Still seen at the same repo/branch: https://github.com/sveltejs/kit/compare/master...kjmph:feature/event-source?expand=1

We'll open a PR referencing this issue after a final review by @jesseskinner.

RE: @juanmait; while I understand the comment, I think this issue is focused on using the same function signature with a different returned object to support streaming through async iterators. The suggestion to add the request and/or response object into the function signature would change the design of endpoints in SvelteKit. Perhaps you should open a new issue to address that?

juanmait commented 2 years ago

RE: @juanmait; while I understand the comment, I think this issue is focused on using the same function signature with a different returned object to support streaming through async iterators. The suggestion to add the request and/or response object into the function signature would change the design of endpoints in SvelteKit. Perhaps you should open a new issue to address that?

Having some support is very welcome. I'm wondering if this solution will fit in an environment like cloudflare workers. The stream API is different from nodejs, it's basically the same API that ships in modern browsers. I'll try this option anyways in nodejs. Thanks!

istarkov commented 2 years ago

Access to raw response object looks like must have.

Async iterables are cool, but could cause some hard to find issues, like you can't just write to a stream i.e.

for await (const event of rendered.body) {
   res.write(event);
}

you need to handle backpressure in this case, end etc events. All needed processing can be achieved easily by using pipeline from 'node:stream' but in case of event streams flush is needed.

PS: Probably idea as at micro framework, you have access to raw response but can just return result see comment (// Send value if it is not undefined, otherwise assume res.end)

kjmph commented 2 years ago

Hello @istarkov, a few points to address in regards to your comment. I'll break them out into a numbered list.

  1. Should request/response be accessible to the endpoint handler? I believe this has been discussed via #334. That's not the objective for this issue.
  2. Attempting to extend the endpoint handler using async iterable objects is precisely what we are experimenting with here.
  3. We should handle back pressure, and we haven't implemented that yet. We will make the change before a pull request is opened.
  4. We believe end events can be handled when the sequence completes or in the case of an infinite sequence, when the connection is destroyed.
  5. Using pipeline from node:stream would ensure this solution would only live in adapter-node. Is there a package that implements the Streams API so this could be more agnostic?
  6. We are currently experimenting with flush via checking the content-type header and using compressible to decide if flushes are required. Works for EventStream.
  7. Micro is an interesting project; yet does not conform with the endpoint design as-is.
kjmph commented 2 years ago

Since you had asked after it @istarkov; completed the back pressure changes. I needed the push from you, since the compression bug was outstanding. :)

https://github.com/sveltejs/kit/commit/38c0d4a2bc51bf7f4a0da05df9a5c6e9b1da3a76

benmccann commented 2 years ago

I think that we will end up exposing the ability to add middleware in adapter-node (https://github.com/sveltejs/kit/pull/2051)

jesseskinner commented 2 years ago

Ok great. Sounds like a much more flexible solution, for now at least.

AndreasHald commented 2 years ago

I think that we will end up exposing the ability to add middleware in adapter-node (https://github.com/sveltejs/kit/pull/2051)

Does this mean that the accompanying pr won't be merged?, because having direct access to add middlewares is awesome and a more simple way to add web sockets.

But isn't the ability to stream responses still valid?

Cloudflare workers also support such use cases, so it doesn't have to be exclusively for adapter-node

benmccann commented 2 years ago

Even if we don't merge https://github.com/sveltejs/kit/pull/2051, I'd still like to find a way to let people add middleware

kjmph commented 2 years ago

My apologies, I've been out of pocket for a while with work. @jesseskinner wanted to add support for adapter-static, which is what delayed this patch. I'll rebase and sort conflicts; then perhaps we can open the pull request and move the conversation there? I concur @AndreasHald, this change still seems useful.

benmccann commented 2 years ago

The proposed PR would support serverless too?

jesseskinner commented 2 years ago

@benmccann yeah, the idea is that all adapters would have to support apps returning an async iterator in the body. Most adapters would do the same as adapter-static and collect as a string. But any adapter could stream the response instead.

mrkishi commented 2 years ago

While async iterators make for a nice interface, it seems things are converging on WHATWG's Streams API for cross-environment streams. They are being used at least on Service Workers, Cloudflare Workers, and Deno. For Node, there's a reference implementation, a pretty popular polyfill and even experimental official support.

What are the reasons to go with async iterators in this case? Unless we're pragmatically picking them, Streams seem like a safer bet for compatibility.

That said...

I understand SvelteKit is supposed to provide a common interface regardless of the underlying platform — but is that a hard requirement or would adapter-specific functionality ever be valid? I ask because in this case we're indirectly dealing with platform details. Users will rarely create streams out of thin air: while they could have generators pumping out data algorithmically, they'll most likely get data from somewhere else, and in that case, they'll get the platform's stream implementation.

In other words, right now, an endpoint using fetch will get either a web or a node stream depending on the deployed platform. Same for reading files. So apps would need platform-specific code to create a platform-agnostic iterator or stream anyway!

What do you think about adapter-provided stream handling? SvelteKit could handle strings, objects, and async iterators out of the box. Adapters could add support for platform-specific types like Node/Web streams and buffers. As long as user code dealt with streams opaquely, it would work regardless of the platform. Unfortunately, if in-flight manipulation was required, they'd need API-specific code.

Unless the idea was to eventually ship with platform-specific wrappers to normalize all IO so that SvelteKit apps are truly environment-agnostic? In that case, an adapter-provided fetch would have a SvelteKit Stream/Iterator body, the same type expected of endpoint responses.

jesseskinner commented 2 years ago

@mrkishi Thanks for the feedback! TBQH I chose async iterators just to lean on a widely adopted platform-agnostic API, and I didn't realise the Streams API was so widely supported. Since it sounds like it's now also a platform-agnostic solution, the Streams API would obviously be an ideal API for streaming, so I'd be very much open to changing my proposal accordingly.

As for keeping SvelteKit applications environment-agnostic - yes I believe that is the spirit of the project, that you could easily change your deployment from one serverless platform to another, without changing your implementation. So the closer we can come to being agnostic, the better.

My specific original use case was to create streams "out of thin air" using a streaming csv library, generating a node stream with a CSV response built using files downloaded from S3. Node streams are also async iterators which was why I thought it was a nice solution. But the Streams API would work here as well.

kjmph commented 2 years ago

Async iterators can be converted to streams for adapters that support the Streams API (we could support Cloudflare Workers and I can add that if we want to see it). I don't know the maintainers philosophy, yet I'm hesitant to suggest a feature that is dependent on non-LTS versions of Node.js or dependencies that would be foisted on downstream developers. Plus, where Node.js 16.6.2 has experimental support, the ReadableStream is still converted to a Node Stream via async iterator support to send the ServerResponse.

I am not a maintainer, but I think the SvelteKit design is one that the endpoints are emitting data, and the async iterator promotes that design choice. Regardless of my opinions, here are two demonstrative endpoints for comparison. (Tested on Node.js 16.6.2 on experimental branches in my fork)

Streams API:

import type { RequestHandler } from '@sveltejs/kit';
import type { Locals } from '$lib/types';
import { ReadableStream } from 'node:stream/web';

const wait = ms => new Promise(resolve => setTimeout(resolve, ms));

// GET /es.json
export const get: RequestHandler<Locals> = async (request) => {
    // Handle setup here..
    const es = new ReadableStream({
        async start(controller) {
            while (true) {
                controller.enqueue(`event: time\ndata: ${(new Date()).toISOString()}\n\n`);
                await wait(1000);
            }
        },
        cancel() {
            // Handle cleanup here..
        }
    });

    return {
        headers: {
            'Cache-Control': 'no-cache',
            'Connection': 'keep-alive',
            'Content-Type': 'text/event-stream',
        },
        body: es
    };
};

Async Iterators:

import type { RequestHandler } from '@sveltejs/kit';
import type { Locals } from '$lib/types';

const wait = ms => new Promise(resolve => setTimeout(resolve, ms));

// GET /es.json
export const get: RequestHandler<Locals> = async (request) => {
    // Handle setup here..
    async function* es() {
        try {
            while (true) {
                yield `event: time\ndata: ${(new Date()).toISOString()}\n\n`;
                await wait(1000);
            }
        } finally {
            // Handle cleanup here..
        }
    }
    return {
        headers: {
            'Cache-Control': 'no-cache',
            'Connection': 'keep-alive',
            'Content-Type': 'text/event-stream',
        },
        body: es()
    };
};

The only caveat is that the import of ReadableStream from node would have to be injected by SvelteKit into the global namespace per adapter. I don't know how to accomplish that. I suppose this is another reason why I like async iterators, they are more universally available today..

benmccann commented 2 years ago

the import of ReadableStream from node would have to be injected by SvelteKit into the global namespace per adapter. I don't know how to accomplish that

We do that with fetch already. See:

Rich-Harris commented 2 years ago

@mrkishi good to see you! long time.

Just chiming in to say that I think using async iterators rather than ReadableStream is ultimately the right choice, for a couple of reasons:

chanced commented 2 years ago

It really seems like a lot of effort is going into making svelte-kits capabilities uniform across the spectrum of adapters. I understand that, to a degree. It makes documenting and migration easier. I think an important question is, what do you lose by doing so?

In reading through this issue and related (people requesting middleware or file uploads), I'm sort of left with confusion over what Svelte Kit aims to be. @Rich-Harris, you've called SvelteKit a "serverless framework" and have voiced opposition to exposing the underlying connect-style req / res from polka in the node-adapter because you're concerned people will default to using it over alternatives due to its capabilities.

This makes sense if the objective of the project is to be a serverless framework. I totally understand why, in that context, that you'd rather people abandon node and serverful environments. It's just another thing that needs to be maintained and a deviation from the goal, a distraction.

If the objective is for SvelteKit to be agnostic of where a person's application resides, be that a kubernetes cluster, netlify, vercel, or the next trend, then I think you may want to consider how it can be perceived from folks who are not going with serverless environments. It seems as though you are limiting yourself to the lowest common denominator while devoting energy to construct work arounds which conform to that standard.

I understand that some of the defacto packages in the node ecosystem can be confusing or less than desirable. In fact, it is somewhat surprising to me that many haven't been supplanted over the years. Having said that, they are clearly still viable and heavily depended upon. This means that adoption of kit will add on even more development tax of using Svelte as people are forced to re-invent the wheel with less resources and capabilities than are present in their stack.

I think it may also limit folks to address kit-specific challenges. For example, I'd like to be able to generate static files, house them in GCS, and stream them down upon request. That's not feasible with kit as-is.

The usage of async generators may make part of that problem solvable but it'll require more effort on my part than utilizing node streams today or web streams in the future.

I'm not sure I would have chosen Svelte and Kit, despite my opinion that Svelte is the best option available for front-end frameworks, if my API weren't being built in another language. I would have probably rolled with Next and just dealt with my dislike of React.

Rich-Harris commented 2 years ago

The goal is to make SvelteKit apps as portable as possible, which definitely doesn't preclude adding non-portable things like direct access to underlying primitives like res eventually. I do think it's worth holding off as long as possible so that we're able to clearly articulate the problems we're trying to solve and see how well we can solve them in a portable way.

This thread is a wonderful example of that. If res was freely available, people would have just used it for streaming responses, and we wouldn't have #2212 which to my mind is a better solution.

It's definitely tempting to think 'if we have the escape hatch, not only will we be able to solve $THIS_PROBLEM, we'll be able to solve a bunch of $OTHER_PROBLEMS in future, but what I've often found is that when you actually sit down and think about $OTHER_PROBLEMS it turns out to be a finite list where we already know the general shape of the solutions; we just have to implement them. Sometimes there are problems you couldn't anticipate, and in those cases escape hatches are useful, but it does need to be an escape hatch rather than a blessed solution otherwise it ends up being used for everything.

The consequence of relying on res isn't just that code becomes less portable (though that is very much the case). It's that Node becomes the default way to use SvelteKit, because the ecosystem hardens around existing middleware packages etc. Ultimately that will just make it harder to use things like Cloudflare Workers and Deno Deploy, even though they will likely have better cost/performance characteristics for many apps. (Case in point: if you had rolled with Next, those platforms wouldn't be an option for you.) I think that would be a shame. Node is the present but it isn't the future (at least, it won't enjoy its current monopoly on server-side JS), and so I think we want to avoid being Node-centric in our assumptions.

The usage of async generators may make part of that problem solvable but it'll require more effort on my part than utilizing node streams today or web streams in the future.

Could you elaborate on this part? You would be able to use node streams like so...

return {
  body: fs.createReadStream('data.csv')
};

...and web streams like so:

// today
return {
  // https://jakearchibald.com/2017/async-iterators-and-generators/#making-streams-iterate
  body: streamAsyncIterator(readable)
};

// tomorrow
return {
  body: readable.getIterator()
};
chanced commented 2 years ago

I'm going to back-peddle on my reply above a bit. Both Fastify and Koa departed from the traditional connect/express approach to server middleware and did just fine. Micro, the server for Next, is different. In the case of Koa and Micro, middleware could be wrapped to work. Fastify has done quite well despite the fact that node's legacy middleware are all but incompatible and require ports, such as fastify/fastify-passport.

Rich-Harris commented 2 years ago

My hope is that many use cases could be addressed via an adapter that converts Connect middleware, not unlike how serverless-http mocks req/res objects so you can use them inside Lambda event handlers. It's exactly the kind of grotesque mad science that lets you ship today, but makes you feel sufficiently grubby that you're motivated to adopt (or build) a leaner, more idiomatic solution:

// src/hooks.js
import { sequence } from '@sveltejs/kit/hooks';
import { convert } from 'svelte-kit-middleware-adapter';
import foo from 'express-foo';
import bar from 'express-bar';

export const handle = sequence(
  convert(foo()),
  convert(bar()),
  ({ request, resolve }) => resolve(request)
);
chanced commented 2 years ago

Sorry, I posted my backtrack above without seeing that you had responded to me.

It's definitely tempting to think 'if we have the escape hatch, not only will we be able to solve $THIS_PROBLEM, we'll be able to solve a bunch of $OTHER_PROBLEMS in future, but what I've often found is that when you actually sit down and think about $OTHER_PROBLEMS it turns out to be a finite list where we already know the general shape of the solutions; we just have to implement them.

Thats certainly true. It'll slow adoption considerably but perhaps thats not a bad thing. If the framework provides viable mechanisms to employ the logic, its just a matter of time before the solution bank is built up. It puts more on early adopters but that's always the case.

Could you elaborate on this part? You would be able to use node streams like so...

I stand corrected. That'll be clean enough for me.

My hope is that many use cases could be addressed via an adapter that converts Connect middleware, not unlike how serverless-http mocks req/res objects so you can use them inside Lambda event handlers.

That'll work.

Thanks for taking the time to explain your stance and reasoning. And for Svelte/Kit.

kjmph commented 2 years ago

I’m glad to see the alignment solidifying. I concur that canceling the timeout is a fairer comparison, if a more robust example was helpful, we could move on that.. I must make the callout that I’m running a company and could use any assistance on getting that pull request over the hump; otherwise I’ll continue to be fitful with progress. In what way can I be the most helpful?

jesseskinner commented 2 years ago

@kjmph No worries, I was hoping to give it a shot this week, either rebasing your branch or maybe starting a new branch if it's easier, and try to get this done soon. Will keep you all posted.

EDIT: hadn't seen that you already dealt with rebasing in the new PR - awesome, thanks!

jesseskinner commented 2 years ago

And @Rich-Harris big thanks for giving this your feedback and blessing, it's exciting to be reassured we're on the right track with this approach.

mrkishi commented 2 years ago

Great discussion, everybody.

I think I should've skipped the first part of my comment as I realized midway async iterators were a good default, but ended up only mentioning it in passing.

After testing some async iterators behavior, I'm wary about the stream to async iterator to stream conversion that's going to be necessary on Node/Deno/Web when doing IO. There's a subtle but important mismatch in that async iterators don't support cancellation. I don't think we can guarantee cleanup in the general case. Check out people trying to deal with cancellation and how finicky it can be. I'm not sure how to deal with that.

Note that it'd be possible to leak resources even without crazy pipelines:

const req = await fetch('/sparse-stream');
return { body: streamAsyncIterator(req.body) };

When the downstream connection closes, at most we could call body_iterator.return(), but that wouldn't propagate to fetch until the next chunk of data arrives. If they're infrequent updates, the orphaned connections would just accumulate until timeout. This isn't too serious, but I'm traumatized by Node Streams and wouldn't be surprised to find worse edge cases.

jesseskinner commented 2 years ago

@mrkishi good point, there would be no way to know a connection was closed and thus no opportunity to abort and release resources early other than timeouts. Note this would also be the case if you tried to create a massive string response asynchronously - even if the connection is lost, the async endpoint will continue working away. So you have a good point, that async iterators are an imperfect and incomplete proxy for streams, and so hooking into the req & res objects would allow listening to events and saving resources. But I think async iterators will end up being good enough most of the time, certainly for my use cases.

andykais commented 2 years ago

async iterators don't support cancellation

@mrkishi @jesseskinner this could probably be solved by providing an AbortController inside api handlers. AbortControllers are web agnostic so I doubt the break the existing goals of sveltekit https://developer.mozilla.org/en-US/docs/Web/API/AbortController. Psuedocode would look like this:

import type { Request } from '@sveltejs/kit'

export async function get(params: Request) {
  const { signal } = params.controller
  // sveltekit will call controller.abort() if the request connection is closed
  const external_resource = fetch('https://example.com', { signal })
  return {
    body: streamAsyncIterator(external_resource.body)
  }
}

Internally, sveltekit would just have to make sure it returns the iterator before cancelling the AbortController. This would mean manually passing around abort controllers, but I think this is actually the ideal scenario. It means we have full control over which resources we want to close with request and which we want to keep open.

Imo its totally fine waiting to implement this separately from async iterator support, but there might be other opinions there.

andykais commented 2 years ago

Just a follow question because I may have misunderstood the use case here. If I want to serve video files to the browser, would I be able to use iterators to stream that data or would I have to write an endpoint that understands http-range headers? I can see iterators or streams serving data from 0 -> content-length, but if a user skips around a video it seems like streams could not help here.

kjmph commented 2 years ago

RE: AbortController; I fully believe this lines up with the SvelteKit portability goals, yet I highly suggest we look at that independently from this issue. That's because converting from a readable stream to an iterator is introducing the need for cancellation. Maybe that is an edge case better solved with middleware through some of the other proposed PRs?

RE: http-range; usage of an iterator will just conveniently allow us to stream data from the normal endpoint handling. You are correct that one would need a custom endpoint that understands http-range headers to decide where to start the stream from. I believe createReadStream does support start and end options for this purpose. This would necessitate cancellation support.

quentincaffeino commented 2 years ago

I created a separate issue about streaming requests on endpoints but it was closed by maintaner. Does this issue mention this use case? If not then here's my input on data streaming https://github.com/sveltejs/kit/issues/2992

kjmph commented 2 years ago

That's correct, this issue focuses on responses. Requests are limited to parsing in some restricted cases, as the documentation clarifies. Here is an example of the runtime, and a Cloudflare Worker Request Context constructing the rawBody. There is an expectation that streams aren't a valid input for rawBody. I'd suggest clarifying that in your linked issue @quentincaffeino, perhaps it can be re-opened and investigation can commence on if it is feasible to stream a request body.

benmccann commented 2 years ago

Hi folks, I wanted everyone to know that Rich posted a mini-RFC regarding file uploads, which overlaps quite a bit with the discussion here around async iterators, etc. Please take a look

Rich-Harris commented 2 years ago

I've opened #3419 which covers the use case discussed here as well as multipart forms (#70), in light of the changes in #3384. tl;dr @mrkishi convinced me that ReadableStream is preferable even if the ergonomics aren't quite as nice, and now that we've switched to returning standard Response objects from handle, it makes sense for handlers to return a valid Response body, which means ReadableStream.

Closing this in favour of that issue — thanks for the valuable discussion everyone!