feat: inline fetch response.body data to page

[ximus]

The general topic is about widening the support of kit's magic fetch in load functions. Currently most response accessors are supported by kit (.json(), .text(), .arrayBuffer()), this PR is about the .body response accessor.

This PR follows up from recent PR #10535

The previous PR linked above added support for inlining fetch response.arrayBuffer() data on the page.
This PR adds support for inlining fetch response.body.getReader() data on the page.

Much of the groundwork of using base64 was laid by @Elia872 in his previous PR.

My app's API client library handles both long running streams (such as messages in a chat widget) and standard run-of-the-mill api calls via a single call to response.body.getReader(). It's a relatively popular library in the small world of GRPC clients, you can see it here.

Long running streams shouldn't be invoked in SSR and this PR doesn't support that. But any short lived streamed data stream fetched via .body could also be inlined in the page, much like is already done for the other response.arrrayBuffer/text/json/...() methods.

Pros:

• ever more support for de-duplicating fetch calls in loads

Cons:

• it's likely a narrow usecase
  • most people use .json(), some .text(), few .arrayBuffer() and scant .body

I skipped creating an issue because I already have this code lying around, I'm using it today in my app.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6a0a580

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ximus]

any concerns about memory? response data will be copied to two locations in memory if I understand this right.

[ximus]

a is guaranteed to close (done === true) before b. So there should not be any race conditions or deadlocks.

Unless someone queries in SSR a long running endpoint which doesn't close. Most people should know not to do that. Let me know if you think we should try to add a guard against that.

[benmccann]

I edited the PR description to put the meaty stuff above the boilerplate. Letting you know as sometimes when it's below we don't see it and the PR ends up sitting longer because no one has context around it

[Rich-Harris]

Thank you — on the whole this seems reasonable (the memory thing does make me a tiny bit nervous but people really shouldn't be making giant requests in this context so in practice it's probably fine). One wrinkle: I think response.bodyUsed will be true in this (admittedly somewhat contrived) situation where it would normally be false:

const response = await fetch(url);
const body = response.body; // causes the body to be cloned, and reading to start
await Promise.resolve();
console.log(response.bodyUsed); // true, even though we haven't touched `body` yet!

Any smart ideas on how to address that?

[ximus]

@Rich-Harris Good catch. I spent a bit of time looking for clever, idiomatic ways using the fetch and stream APIs, but I could not find a way to keep bodyUsed false.

• Using Response.clone() implies tee'ing, so nothing to see here.
• I tried piping streams around in various ways but it's impossible to both kick off the the stream read, and not "disturb" the .body stream.

What does seem to work is delaying this same previous tee'ing implementation one call further, to be invoked when methods on .body get called. Specifically methods that lock and read the stream such as .body.getReader, .body.pipeTo, etc ...

Here is what seems to work, it requires adding another proxy, this time around body.

if (key === 'body') {
  const body = response.body;
  if (!body) return body;

  /**
   * @param {ReadableStream} body
   */
  function tee_and_buffer_body(body) {
    const [a, b] = body.tee();
    let buffer = new Uint8Array();
    const reader = a.getReader();
    /**
     * @param {{
     * 	done: boolean
     * 	value?: Uint8Array
     * }} opts
     */
    function buffer_to_fetched({ done, value }) {
      if (done) {
        if (dependency) {
          dependency.body = new Uint8Array(buffer);
        }
        push_fetched(b64_encode(buffer), true);
      } else if (value) {
        const newBuffer = new Uint8Array(buffer.length + value.length);
        newBuffer.set(buffer, 0);
        newBuffer.set(value, buffer.length);
        buffer = newBuffer;
        reader.read().then(buffer_to_fetched);
      }
    }
    reader.read().then(buffer_to_fetched);
    return b;
  }

  /** @type {ReadableStream} */
  let teedBody;

  return new Proxy(body, {
    get(body, key, _receiver) {
      if (
        key === 'getReader' ||
        key === 'pipeThrough' ||
        key === 'pipeTo' ||
        key === 'tee'
      ) {
        teedBody = teedBody || tee_and_buffer_body(body);
        return teedBody[key].bind(teedBody);
      }
    }
  });
}

This exercise also made me realize a second problem with this first implementation, calling .body twice on the same response would trigger ReadableStream is locked because you can't tee the same body/stream twice.

It's not much prettier in terms of code, but I think still acceptable. You tell me.

[Rich-Harris]

we-have-to-go-deeper-jpg

Unfortunately, this still doesn't quite solve it, because you could do reader = response.body.getReader() without calling reader.read(), and response.bodyUsed should still be false (but will be true, because calling getReader sets things in motion).

It's going to end up being a game of whack-a-mole and I don't know that it's worth it to be honest. It feels like enough of an edge case that it's probably fine? (Famous last words.)

Handling the double .body thing is worthwhile though I think.

[ximus]

@Rich-Harris You're right, it doesn't quite solve it, I should have noticed.
I also think it's enough of an edge case. I can hardly think of code needing to branch off bodyUsed.

re: double .body thing: I added the teed_body variable to avoid re-tee()ing on subsequent body calls of the same response object.

[eltigerchino]

I've marked this as draft for now, but feel free to mark it as ready when you'd like someone to review it.

[ximus]

thanks @eltigerchino , this is actually ready for review, I guess I didn't make that clear in my last message. When I say "it doesn't quite solve it", it relate to the issue of bodyUsed being set, but that is considered acceptable as-is.

[Rich-Harris]

preview: https://svelte-dev-git-preview-kit-11473-svelte.vercel.app/

this is an automated message