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

Add Response.json static method #1392

Merged
merged 16 commits into from
May 18, 2022
Merged

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Jan 30, 2022

This commit adds a Response.json static method that can be used to
create well formed JSON responses will very little effort.

The JSON response is not pretty printed.

Closes #1389



Preview | Diff

This commit adds a `Response.json` static method that can be used to
create well formed JSON responses will very little effort.
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Jan 31, 2022

Note that we'd need to fix Web IDL first to allow regular and static operations with the same name:

The identifier of a static operation also must not be the same as the identifier of a regular operation defined on the same interface.

from https://webidl.spec.whatwg.org/#idl-operations

@lucacasonato
Copy link
Member Author

@domenic Do you think that is desirable to change? It may be confusing to have both a prototype and static method with the same name. Maybe this method should be called fromJSON.

@domenic
Copy link
Member

domenic commented Feb 1, 2022

I think it is desirable to change and do not see anything confusing about having such methods, personally.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks reasonable, but I think it can be improved by making initialize a response also take a body and Content-Type header value (potentially as a tuple?). That would allow for sharing even more steps.

I also wonder what we should do here if browser interest isn't forthcoming. Perhaps there's a way in which we can still have mutual agreement, but it's not exposed in browsers for now. Perhaps this is worth bringing up in the HTML triage call (if it doesn't otherwise get resolved).

@jasnell
Copy link

jasnell commented Mar 8, 2022

Just fyi... I have an implementation of this in Cloudflare Workers just waiting on this to land or at least signal that it's moving forward.

@domenic
Copy link
Member

domenic commented Mar 8, 2022

I don't think we have any signal that this is moving forward in browsers, unfortunately, due to it just being low-priority sugar for browser teams. As such I don't think it'll land in Fetch, at least until a couple of browsers have engineers looking for starter projects.

I'd really encourage you to use this as a case study for https://github.com/whatwg/js-hosts . Figure out what sort of documentation you want to create there, which would enable you to land such things with confidence, and go for it! I'm happy to participate and secure guarantees like "Chromium will never ship a Response.json() with different semantics than those specified here".

@lucacasonato
Copy link
Member Author

@domenic Would this get a 👍 from Chromium if we did the implementation work?

@domenic
Copy link
Member

domenic commented Mar 8, 2022

@ricea would be the decider there but I can predict with very high probability he would support it :).

fetch.bs Show resolved Hide resolved
@lucacasonato lucacasonato requested a review from annevk March 9, 2022 18:07
@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 10, 2022
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Mostly nits and request for a follow-up. Shared algorithm looks rather good! Thanks.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
annevk pushed a commit to web-platform-tests/wpt that referenced this pull request May 18, 2022
@annevk annevk added addition/proposal New features or enhancements topic: api labels May 18, 2022
@annevk annevk merged commit b3bfd0c into whatwg:main May 18, 2022
@foolip
Copy link
Member

foolip commented May 23, 2022

This change isn't possible to roll into WPT's idlharness.js test, see web-platform-tests/wpt#34155. Does anyone want to give it a go to update idlharness.js to avoid this problem?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 24, 2022
…stonly

Automatic update from web-platform-tests
Fetch: add tests for Response.json

Context: whatwg/fetch#1392.
--

wpt-commits: 27e770bcfbeae534b5d4cbc0a50d2b8247942312
wpt-pr: 32825
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request May 25, 2022
…stonly

Automatic update from web-platform-tests
Fetch: add tests for Response.json

Context: whatwg/fetch#1392.
--

wpt-commits: 27e770bcfbeae534b5d4cbc0a50d2b8247942312
wpt-pr: 32825
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 30, 2022
This commit adds support for the `Response.json` static method as
specified in whatwg/fetch#1392.

All WPTs from web-platform-tests/wpt#32825 pass.

Bug: 1305358
Change-Id: Iaafdd514ed12644b6433c02e7d076ce43f51b9ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3639064
Reviewed-by: Yutaka Hirano <[email protected]>
Reviewed-by: Adam Rice <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019479}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This commit adds support for the `Response.json` static method as
specified in whatwg/fetch#1392.

All WPTs from web-platform-tests/wpt#32825 pass.

Bug: 1305358
Change-Id: Iaafdd514ed12644b6433c02e7d076ce43f51b9ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3639064
Reviewed-by: Yutaka Hirano <[email protected]>
Reviewed-by: Adam Rice <[email protected]>
Commit-Queue: Yutaka Hirano <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1019479}
NOKEYCHECK=True
GitOrigin-RevId: e680cd1bf0f99e44102f8714dcb5f77fba82c5ed
webkit-commit-queue pushed a commit to andreubotella/WebKit that referenced this pull request Mar 22, 2023
https://bugs.webkit.org/show_bug.cgi?id=240375

Reviewed by Youenn Fablet.

This implements the `Response.json` static method, added to the fetch
spec in whatwg/fetch#1392.

* LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.worker-expected.txt:
* Source/WebCore/Modules/fetch/FetchBody.h:
* Source/WebCore/Modules/fetch/FetchResponse.cpp:
(WebCore::FetchResponse::create):
  Added this method overload to implement the spec's "initialize a
  response" algorithm. This algortihm used to be combined with the
  regular Response creation algorithm which extracts the body, but
  Response.json cannot use that directly.
(WebCore::FetchResponse::staticJson):
* Source/WebCore/Modules/fetch/FetchResponse.h:
* Source/WebCore/Modules/fetch/FetchResponse.idl:

Canonical link: https://commits.webkit.org/261960@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: api
Development

Successfully merging this pull request may close these issues.

Proposal: Response.json helper
8 participants