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

feat: add static Response.json #1670

Merged
merged 2 commits into from Nov 10, 2022

Conversation

KhafraDev
Copy link
Contributor

Purpose

Implements static Response.json from whatwg/fetch#1392

Changes

Adds static Response.json

Additional information


  • I updated readme
  • I added unit test(s)

@@ -124,6 +124,25 @@ export default class Response extends Body {
return response;
}

static json(data = undefined, init = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that using init = {} and assuming init is an object further down in the function will make the following code crash:

Response.json({ hej: 1 }, null)

But in Chrome that works:

Screenshot 2022-11-09 at 09 22 54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node-fetch doesn't seem to do much argument validation, I followed other areas of the code.

For example when constructing a new response:

new Response('', null)

throws an error when it shouldn't

new Response('', 3)

works when it shouldn't (same with booleans, etc)

src/response.js Outdated Show resolved Hide resolved
src/response.js Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator

I'm okey with some small deviations, we don't have all the webidl stuff undici have and don't follow the spec at 100%. it was set up to be "good enough" and solve the most common use cases.
We didn't have all the web parts in NodeJS that builds up fetch for what it is today in the beginning when node-fetch was first built, like web streams, blob, formdata, AbortController, etc. so we did compromise a little bit.

it's going to be replaced by NodeJS in the newer versions anyway at some point. node-fetch is just a polyfill at this stage.
it at least gets the job done and i'm sure ppl will get it right 99% of the times.

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@jimmywarting jimmywarting merged commit 55a4870 into node-fetch:main Nov 10, 2022
@github-actions
Copy link

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impl "Response.json"
3 participants