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

Response missing body property #530

Closed
lukeed opened this issue Jul 10, 2022 · 10 comments · Fixed by #1255
Closed

Response missing body property #530

lukeed opened this issue Jul 10, 2022 · 10 comments · Fixed by #1255

Comments

@lukeed
Copy link

lukeed commented Jul 10, 2022

Version

0.1.2

Platform

Darwin MacBook-Pro.local 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan 5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T6000 arm64

What steps will reproduce the bug?

The body property doesn't seem to exist on the Response class, which means that an existing Response cannot be cloned (and made mutable) via new Response(res.body, res):

export default {
  port: 8080,
  async fetch(req: Request) {
    let res = new Response('hello world', { status: 404 });
    console.log('first:', res);
    let copy = new Response(res.body, res);
    console.log('copy:', copy);
    return copy;
  }
}

How often does it reproduce? Is there a required condition?

Every request

What is the expected behavior?

The entire Response should be cloned into a new, mutable Response instance that includes the original's body content. In this case, the response should be 404 with "hello world" text body.

What do you see instead?

Response is 404 with no body content and Content-Length: 0 header.

Additional information

console output:

first: Response (0.01 KB) {
  ok: false,
  bodyUsed: false,
  status: 404n,
  url: "",
  statusText: "",
  redirected: false
}
copy: Response (0 KB) {
  ok: false,
  bodyUsed: false,
  status: 404n,
  url: "",
  statusText: "",
  redirected: false
}

Note: wrt the spec, status shouldnt be a bigint

@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Jul 10, 2022

Web Streams for fetch is not implemented yet, which would include Response here (and Request)

For now, if you just want to get a ReadableStream for a Response object:

const stream = (await response.blob()).stream();

Note: wrt the spec, status shouldnt be a bigint

This is a console.log formatting bug where it sometimes adds the n when it should not.

@lukeed
Copy link
Author

lukeed commented Jul 10, 2022

I think I follow, but just to be sure, you're saying that ReadableStream (and co) aren't fully implemented yet but will be done & align with spec soon?

Appreciate the workaround, but looking for CFW/Deno compatibility. This seems to be the only blocker for my worktop framework to work with Bun :)

@Jarred-Sumner
Copy link
Collaborator

I think I follow, but just to be sure, you're saying that ReadableStream (and co) aren't fully implemented yet but will be done & align with spec soon?

Yes. fetch internally needs some changes to the implementation to better support ReadableStream, along with HTMLRewriter (the other type that uses Response). This will be added, just not yet

Appreciate the workaround, but looking for CFW/Deno compatibility. This seems to be the only blocker for my worktop framework to work with Bun :)

awesome!

@piliugin-anton
Copy link

Web Streams for fetch is not implemented yet, which would include Response here (and Request)

For now, if you just want to get a ReadableStream for a Response object:

const stream = (await response.blob()).stream();

Note: wrt the spec, status shouldnt be a bigint

This is a console.log formatting bug where it sometimes adds the n when it should not.

@Jarred-Sumner It will load the whole request body into a memory, right?

@Jarred-Sumner
Copy link
Collaborator

@Jarred-Sumner It will load the whole request body into a memory, right?

yes

@piliugin-anton
Copy link

@Jarred-Sumner Can I ask unrelated question here? Do you know why Bun.serve starts listening exactly after 4 seconds (Ubuntu 22.04)? Is it a bug?

@Jarred-Sumner
Copy link
Collaborator

It’s an event loop bug. The task to sleep the event loop to wait for the next request needs to happen outside the microtask queue. Right now there is one queue for tasks of all kinda

@piliugin-anton
Copy link

piliugin-anton commented Jul 10, 2022

Okay, thank you for response. Thank you for your great work! Current workaround for getting a request stream is not suitable for file uploads, so I will wait while Request/Response streams will be implemented

@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Jul 10, 2022 via email

@piliugin-anton
Copy link

piliugin-anton commented Jul 10, 2022

I think you’ll really like the solution in mind for file uploads. The same syscall that lets bun serve files at 5.8 GB/s could work for uploading them (as long as http only, no client-side compression) https://twitter.com/jarredsumner/status/1506266502088957953?s=21&t=S_xRebD_ZiQyZCpNvGUPtg

Awesome! It will not work for SSL enabled servers?

gornostay25 referenced this issue in gornostay25/svelte-adapter-bun Jul 10, 2022
gornostay25 referenced this issue in gornostay25/svelte-adapter-bun Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants