Reland JSON module scripts by dandclark · Pull Request #5658 · whatwg/html (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation81 Commits71 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Reland JSON modules, originally authored by @littledan in db03474 and now proposed in TC39 at https://github.com/tc39/proposal-json-modules. Importing a JSON module now requires the module type to be specified at every import site with an import assertion, addressing the security concern that led to the revert.

At least two implementers are interested (and none opposed):
- Chromium: Shipped in 91
- Firefox: Worth Prototyping. Note, the link's title is "Import Attributes" but also reflects Mozilla's position on JSON modules. See [1] and [2].
Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/tree/master/html/semantics/scripting-1/the-script-element/json-module
Implementation bugs are filed:
- Chrome: http://crbug.com/1132413
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1670176
- Safari: https://bugs.webkit.org/show_bug.cgi?id=228307

/infrastructure.html ( diff )
/scripting.html ( diff )
/webappapis.html ( diff )

As discussed in #5640, I think we need to make the import type part of the module map key. That will also avoid the awkward exception to caching null.

Reland JSON modules by reverting a530f6f, resolving merge conflicts, and updating the change with respect to the string changes in a9ef15d.

Update all [[RequestedModules]] references to reflect the change from string to ModuleRequest. Pass ModuleRequest instead of url to 'internal module graph fetching procedure'. Add optional ModuleRequest param to 'fetch a single module script'. Add checks in 'fetch a single module script' to fail if the type doesn't match.

…module script graph', HostResolveImportedModule, and HostResolveImportedModuleDynamically.

… type is valid but doesn't match the requested type.

…pecify type at the point of preload. Achieve this by adding 'module type must match' flag to 'fetch a single module script', which is unset only in the case of modulepreload.

Choose a reason for hiding this comment

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

This patch looks great. I agree with all of the design decisions, and just have a couple editorial comments.

@domenic To clarify, the import attributes proposal isn't about what you're importing but whether the import meets certain conditions. For this reason, we switched the token introducing this form from with to if and are considering renaming the proposal to import conditions. Modifying what is being imported (which would logically be part of the cache key) would be a separate proposal, if sufficient use cases are identified. We're working on clarifying this distinction in our documentation and hope to present on it in the upcoming TC39 meeting.

I can see how the error handling in this patch differs from other errors which are cached as null in the module map, but I don't see why this is a problem, given that the error here is about the import site.

source Outdated Show resolved Hide resolved

If we ever: (a) get more Synthetic Module Record Users; or (b) start testing if something is a
JSON module script in algorithms, instead of just referring to the concept, then we should
consider adding a type item to the module script struct.
-->

Choose a reason for hiding this comment

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

I agree with this comment--the current representation seems fine for now. If we want something more explicit, I'd suggest that module scripts have the additional struct item of 'type', which could be either "json" or undefined (at first).

Choose a reason for hiding this comment

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

I've updated this comment to reflect that we now have more than one Synthetic Module Record user (CSS and JSON module scripts). We don't have anything that switches on module type that I know of though, so I think a module type item in the struct is still not needed.

source Outdated Show resolved Hide resolved

@littledan I don't think that addresses my concern. I would like to factor this specification so that errors continue to be cached, and the best way to do that is by making the type part of the module map key.

@domenic Thanks for your reviews; I'd be interested in understanding this in more detail. I reopened the issue to discuss module cache keying in the TC39 proposal. Maybe we could continue discussion in the proposal repo, since we'd need a change in the proposal, not just the HTML integration, to make the change you're suggesting.

I'll leave the TC39-side discussions to others, but I've hopefully made it clear what I'd want to see from the HTML integration.

Well, the understanding I'm missing is, why is the non-caching of the type check failure a problem? When we were talking about this logic in the air in #5640, I was imagining that it might be difficult to explain this logic, but this patch seems to demonstrate otherwise.

If (url1, type = X) has failed once, it should always continue to do so. The same way that today if url1 has failed once, it always continues to do so.

For example, we shouldn't allow changing import "./foo.mjs" to import "./foo.mjs" if { type: "javascript" } to bypass the idempotency of imports and start re-querying servers for the same URL multiple times on a single page.

I see; thanks for explaining. What if, in the case of type mismatch for a module which is not in the module map, we cache the entire response body (as a string, without interpreting it) together with the MIME type, to avoid the repeated fetches?

That's a valid implementation strategy, but on the spec side we should just use extended module map keys.

There would be an observable difference between these strategies, that extending the module map keys would result in repeated fetches if the same module is imported with different types, whereas specifying that the response is cached as I described would result in just a single fetch.

Ah, yes. That makes sense then; we definitely want multiple fetches in that scenario, since they are very different import statements.

@domenic Could you say why? The import attributes (or "import conditions") proposal is about specifying checks at the import site to be performed when importing modules; I don't understand why there should be multiple fetches when loading a module with different checks.

Because a failing import with one type value shouldn't impact an import with another type value.

…'; I'll add this in a separate PR.

…defining steps in HTML spec

Hmm, it seems like I haven't explained the idea in #5658 (comment) very clearly. The type check would be per import, not affecting another import site. Maybe better to discuss this further if it can be written up in this PR. (Note, the idea of doing this caching here was first presented by @dandclark offline.)

If we continue caching in the way the HTML spec does, then it would affect another import site. And we should not change how HTML does caching.

I do feel like we're going in circles, but I am hopeful that I've made things clear by now how this feature should integrate into HTML. If you can update the PR to do that, then I'd be happy to continue the review.

Agree that HTML's caching mechanism shouldn't change and that per-module (not import site) caching is the way to go. Also feel that changing the type should be treated as a separate module and refetch. There might even be some use-cases for trying a dynamic import multiple times with different types.

One issue with not making the type, etc. part of the module map key is that with JavaScript the error object is going to be referentially equal with multiple imports and for non-JavaScript it will not: https://gist.github.com/matthewp/8ed6249c433473e3e138e417b009c0e8

The inconsistency seems wrong to me but I can't think of a use-case where I'd be checking that they are the same either. I can also see the point of view that checks are unrelated to the module's identity so it sort of feels wrong in that case to make them part of the key. But I don't feel very strongly about this.

One thing to consider is that if other checks are added using this same mechanism in the future, integrity and crossorigin for example, would those also be desirable to be part of the key?

dandclark changed the title~~Add import assertions integration and reland JSON modules~~ Reland JSON modules

Jul 27, 2021

dandclark marked this pull request as ready for review

July 27, 2021 20:49

The import assertions integration landed separately in #4898, so I've updated this change to merge that out. This PR (much smaller now) is ready for an updated review.

cc @littledan, @annevk, @domenic

Thanks!

dandclark changed the title~~Reland JSON modules~~ Reland JSON module scripts

Jul 27, 2021

Choose a reason for hiding this comment

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

LGTM, only editorial issues!

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Show resolved Hide resolved

Choose a reason for hiding this comment

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

LGTM! Will merge tomorrow unless anyone else spots anything. I recall the tests being pretty exhaustive here.

Do we have tests for UTF-8ness and in particular the UTF-16 BOM not working? That would be good for both here and CSS modules as implementations do get that kind of thing wrong.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request

Jul 29, 2021

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

Bug: 1132413 Change-Id: I306b5deddefc448d2b43f83942c490f2b217a565

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request

Jul 29, 2021

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

Bug: 1132413 Change-Id: I306b5deddefc448d2b43f83942c490f2b217a565 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3059740 Reviewed-by: Kouhei Ueno kouhei@chromium.org Commit-Queue: Dan Clark daniec@microsoft.com Cr-Commit-Position: refs/heads/master@{#906733}

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request

Jul 29, 2021

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

pull bot pushed a commit to jamlee-t/chromium that referenced this pull request

Jul 30, 2021

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request

Jul 31, 2021

…pt filenames, a=testonly

Automatic update from web-platform-tests Remove 'tentative' from JSON module script filenames

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

wpt-commits: 711b29c9ac887e0c72ea2bc16a29dd9b2fd78e24 wpt-pr: 29830

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request

Aug 4, 2021

…pt filenames, a=testonly

Automatic update from web-platform-tests Remove 'tentative' from JSON module script filenames

Remove 'tenative' from JSON module script test filenames now that the HTML spec PR [1] is about to land.

Also delete out-of-date README.

[1] whatwg/html#5658

wpt-commits: 711b29c9ac887e0c72ea2bc16a29dd9b2fd78e24 wpt-pr: 29830

bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull request

May 1, 2025

…type in ModuleScriptFetcher, a=testonly

Automatic update from web-platform-tests Check MIME type against asserted module type in ModuleScriptFetcher

Add module type to the module map key.

Plumb module type into ModuleScriptFetcher and its subclasses, and require that the MIME type match the type that was specified with import assertions.

Add the necessary import assertions to the JSON/CSS module web tests so that they continue passing. Add tests to ensure that the modules don't load when the correct assertion is not present.

A minor functional change to JSON modules is that trying to start a a Worker with a top-level JSON module (e.g. new Worker("./foo.json")) now results in a rejected Promise, instead of loading a no-op worker without an error. This change follows the spec change at whatwg/html#5658; note that the invocation of 'fetch a single module script' from 'fetch a worklet/module worker script graph' doesn't pass a ModuleRequest, so the type is assumed to be JavaScript, and a failure will be triggered if the MIME type is not JavaScript.

Some of the non-virtual versions of the tests now time out because the import assertion syntax is seen as a syntax error when --harmony-import-assertions is not enabled, causing