Skip to content

Conversation

@ekzhang
Copy link
Contributor

@ekzhang ekzhang commented Feb 11, 2023

This is a draft PR since I can't figure out how to compile bun on my computer yet! Will work that out and then run tests to make sure it actually builds and runs.

Resolves #2014

@ekzhang ekzhang marked this pull request as ready for review February 11, 2023 16:46
@ekzhang
Copy link
Contributor Author

ekzhang commented Feb 11, 2023

Update: Okay, just verified it compiles and the test passes!

await fetch(url, { body: "buntastic" });
expect(false).toBe(true);
} catch (exception) {
expect(exception instanceof TypeError);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect(exception instanceof TypeError);
expect(exception instanceof TypeError).toBe(true);

We haven't implemented expect(exception).toBeInstanceOf(TypeError) yet

@Jarred-Sumner
Copy link
Collaborator

Looks good, one nitpicky comment and then I'll merge

@ekzhang
Copy link
Contributor Author

ekzhang commented Feb 12, 2023

Ah it looks like exception instanceof TypeError isn't actually true, and now the test fails.

@ekzhang
Copy link
Contributor Author

ekzhang commented Feb 12, 2023

Any idea what's happening here? I modified the code in response.zig to raise a TypeError using JSC.toTypeError, but now when I run:

fetch("https://www.google.com", { body: "hello" }).catch(err => {
  console.log(err);
  console.log(err instanceof TypeError);
});

I get this output:

$ packages/debug-bun-darwin-*/bun-debug hello.js
1 | fetch("https://www.google.com", { body: "hello" }).catch((err) => {
   ^
TypeError: fetch() request with GET/HEAD/OPTIONS method cannot have body.
 code: "ERR_INVALID_ARG_VALUE"

      at /Users/ezhang/Documents/Work/bun/hello.js:1:0

false

Even though it prints out and displays as a TypeError, the instanceof operator still returns false.

image

Copy link
Contributor

@jwhear jwhear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you!

@jwhear jwhear merged commit 6e1a526 into oven-sh:main Feb 14, 2023
@ekzhang
Copy link
Contributor Author

ekzhang commented Feb 15, 2023

Oh, just FYI the fetch test I added in this PR is still failing @jwhear — not sure if you saw the comment I made above in thread. I was asking for advice on how to resolve that failing test.

@ekzhang ekzhang deleted the ekzhang/fetch-body branch February 15, 2023 21:48
@jwhear
Copy link
Contributor

jwhear commented Feb 15, 2023

Ah, the instanceof TypeError issue; I'll look into it in a bit.

@ThatOneBro
Copy link
Contributor

I think the problem is that we are creating our own TypeError class somewhere which is different from the TypeError that we are using globally. I ran into this before in a different area. I'm thinking some of these TypeErrors are not like the others.

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 this pull request may close these issues.

fetch() should throw TypeError: HEAD or GET Request cannot have a body

4 participants