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 "json" destination, for JSON modules #1691

Merged
merged 4 commits into from
Oct 29, 2023

Conversation

nicolo-ribaudo
Copy link
Contributor

@nicolo-ribaudo nicolo-ribaudo commented Jul 25, 2023

The import attributes TC39 proposal has been updated so that HTML can use the expected type (unspecified, json, css) to affect the fetch request. This PR introduces the "json" destination, that will be used to fetch import "..." with { type: "json" }.

Ref whatwg/html#9486 (comment), whatwg/html#7233

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

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.

Looks great modulo some nits.

I think we can count Chromium and WebKit as supportive based on statements in whatwg/html#7233, but I'll tag some people here just in case.

cc @Constellation @syg @hiroshige-g

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Aug 25, 2023

@nicolo-ribaudo are you planning to write tests for this and implementation bugs?

@hiroshige-g
Copy link

cc/ @domenic

@nicolo-ribaudo
Copy link
Contributor Author

Tests have been merged!

@annevk annevk merged commit 49bff76 into whatwg:main Oct 29, 2023
2 checks passed
annevk pushed a commit to whatwg/html that referenced this pull request Oct 29, 2023
@nicolo-ribaudo nicolo-ribaudo deleted the json-destination branch October 30, 2023 08:22
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 19, 2024
The "Import Attributes" ECMAScript proposal has been updated to allow
attributes to affect how modules are fetched, so that HTML can use them
to set the proper destination when fetching CSS and JSON modules [1].

This has two major effects:
- the `Accept` HTTP header is now specific to the module type, rather
  than simply being `*/*`
- we use the relevant CSP policy rather than always script-src.

This patch only implements this change for CSS modules. The change for
JSON modules will come in a later patch, as it's more complex because it
requires introducing a new fetch destination [2].

It passes the relevant wpt tests [3] when using --js-flags="--harmony_import_attributes.

[1]: whatwg/html#9486
[2]: whatwg/fetch#1691
[3]: web-platform-tests/wpt#41665

Bug: 1491336
Change-Id: I4abf09dd828e5ded2b685be6da231c3fca90e19f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4949956
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Nicolò Ribaudo <nribaudo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1249592}
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 20, 2024
This patch adds a new `json` fetch destination [fetch-spec-pr], that has
the following characteristics:
- it implies the `Accept: application/json,*/*;q=0.5` HTTP header;
- it uses the `connect-src` CSP directive [csp-spec-pr].

This new destination is used when fetching JSON module scripts, using
`import ... from "/data" with { type: "json" }` [html-spec-pr].
https://crrev.com/c/4949956 implements a similar change for CSS module
scripts, but their implementation is simpler because the `style`
destination already exists.

This patch passes all the relevant WPT tests [wpt-pr] (when using
--js-flags="--harmony_import_attributes), although I had to run them
manually because they have not been merged yet.

This patch does not add support for `<link rel="preload" as="json">`,
which is also introduced by the linked fetch and HTML spec changes.

[fetch-spec-pr]: whatwg/fetch#1691
[csp-spec-pr]: w3c/webappsec-csp#611
[html-spec-pr]: whatwg/html#9486
[wpt-pr]: web-platform-tests/wpt#41665

Bug: 1491336
Change-Id: I6661ddc9be04935e2ee760eb78d1060ae0192a55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4955077
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Nicolò Ribaudo <nribaudo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1249822}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants