Reland JSON module scripts #5658

dandclark · 2020-06-19T17:39:48Z

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 )

domenic · 2020-06-19T17:47:32Z

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.

…b1078ae

littledan

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

littledan · 2020-06-22T16:38:51Z

source

+     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.
+    -->


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).

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

domenic · 2020-06-22T17:18:06Z

@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.

littledan · 2020-06-22T18:02:35Z

@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.

domenic · 2020-06-22T18:05:00Z

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.

littledan · 2020-06-22T18:20:10Z

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.

domenic · 2020-06-22T18:25:01Z

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.

…type.

littledan · 2020-06-22T18:48:51Z

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?

domenic · 2020-06-22T19:11:10Z

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

littledan · 2020-06-22T19:21:03Z

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.

domenic · 2020-06-22T19:22:01Z

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

…oposal spec

littledan · 2020-06-22T19:28:07Z

@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.

domenic · 2020-06-22T19:31:26Z

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

littledan · 2020-06-22T20:29:56Z

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.)

domenic · 2020-06-22T20:35:27Z

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.

matthewp · 2020-06-23T11:21:04Z

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.

matthewp · 2020-06-23T14:43:54Z

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 · 2021-07-27T20:53:31Z

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!

domenic

LGTM, only editorial issues!

source

domenic

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

annevk · 2021-07-28T05:19:12Z

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.

domenic · 2021-07-28T18:05:45Z

I don't see such tests. (Edit: there are tests that a UTF-8 encoded file works, and tests that a windows-1250-encoded file decodes as UTF-8 anyway. But there is no test that a UTF-16-with-BOM file ends up as a syntax error, which I think is what you're requesting.)

@dandclark, can you add them? Also a PR to remove the .tentative from all the test filenames, and update https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-script-element/json-module/README.md , would be good.

dandclark · 2021-07-28T21:50:36Z

I'm adding tests for the UTF-16 BOM cases here: https://chromium-review.googlesource.com/c/chromium/src/+/3059241

I'm removing .tenative and the out-of-date README here: https://chromium-review.googlesource.com/c/chromium/src/+/3059740

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

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}

…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 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} -- wpt-commits: 711b29c9ac887e0c72ea2bc16a29dd9b2fd78e24 wpt-pr: 29830

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 <script>s not to run. TestExpectations is updated to account for these. Bug: 1132413 Change-Id: I01bf7d907a96e791208534c9c4f2af11434c76db Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2674259 Commit-Queue: Dan Clark <daniec@microsoft.com> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#852991} GitOrigin-RevId: cc484c290cab98865a656a34a510b023ae405753

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} NOKEYCHECK=True GitOrigin-RevId: adcf773892e6419c4779a2c02c29431ca92847c7

dandclark added 6 commits June 19, 2020 11:26

Reland JSON modules

914e7d5

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

Plumb through ModuleRequest instead of string for 'fetch an import() …

d0137f5

…module script graph', HostResolveImportedModule, and HostResolveImportedModuleDynamically.

Don't cache a null entry in the module map in the case where the MIME…

b2e7065

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

Allow modulepreload to work for all module types without needing to s…

0cbf597

…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.

After rebase, fix build break from name change to JSON.Parse in whatwg@…

6932074

…b1078ae

dandclark force-pushed the dandclark/import-attributes branch from 34933d4 to 6932074 Compare June 19, 2020 18:33

littledan reviewed Jun 22, 2020

View reviewed changes

littledan mentioned this pull request Jun 22, 2020

Module cache keying semantics tc39/proposal-import-attributes#30

Closed

Fix typo in JSON modules example and add note on requiring JSON MIME …

1f83c7e

…type.

Update ModuleRequest definition to reference the import-attributes pr…

a3621dd

…oposal spec

dandclark added 2 commits June 22, 2020 12:31

Revert added null check in 'fetch a modulepreload module script graph…

55ebd12

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

Reference ParseJSONModule from import-attributes proposal instead of …

af41b28

…defining steps in HTML spec

dandclark mentioned this pull request Jun 22, 2020

Check for null before running 'fetch a modulepreload script graph' optional steps #5664

Merged

dandclark added 3 commits July 26, 2021 15:55

Merge branch 'main' into dandclark/import-attributes

ab21bd5

Fix spacing

aa104e0

Delete duplicated example

64c523d

domenic force-pushed the main branch from 0947339 to 1b5099f Compare July 27, 2021 17:07

Restore updated comments about CSS/JSON module script definition

54498ba

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

dandclark changed the title ~~Reland JSON modules~~ Reland JSON module scripts Jul 27, 2021

domenic reviewed Jul 27, 2021

View reviewed changes

source Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Show resolved Hide resolved

dandclark added 4 commits July 27, 2021 14:46

Combine some paragraphs

9de359a

Add missing closing tags

d4f5bd0

Use CreateDefaultExportSyntheticModule for CSS module scripts

43526c4

Use not/or instead of neither/nor

10af46b

domenic approved these changes Jul 27, 2021

View reviewed changes

chromium-wpt-export-bot mentioned this pull request Jul 29, 2021

Remove 'tentative' from JSON module script filenames web-platform-tests/wpt#29830

Merged

domenic added topic: script addition/proposal New features or enhancements labels Jul 29, 2021

domenic merged commit d554d6d into whatwg:main Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland JSON module scripts #5658

Reland JSON module scripts #5658

dandclark commented Jun 19, 2020 •

edited by pr-preview bot

Loading

domenic commented Jun 19, 2020

littledan left a comment

littledan Jun 22, 2020

dandclark Jul 27, 2021

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020 •

edited

Loading

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

matthewp commented Jun 23, 2020

matthewp commented Jun 23, 2020

dandclark commented Jul 27, 2021

domenic left a comment

domenic left a comment

annevk commented Jul 28, 2021

domenic commented Jul 28, 2021 •

edited

Loading

dandclark commented Jul 28, 2021

Reland JSON module scripts #5658

Reland JSON module scripts #5658

Conversation

dandclark commented Jun 19, 2020 • edited by pr-preview bot Loading

domenic commented Jun 19, 2020

littledan left a comment

Choose a reason for hiding this comment

littledan Jun 22, 2020

Choose a reason for hiding this comment

dandclark Jul 27, 2021

Choose a reason for hiding this comment

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020 • edited Loading

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

littledan commented Jun 22, 2020

domenic commented Jun 22, 2020

matthewp commented Jun 23, 2020

matthewp commented Jun 23, 2020

dandclark commented Jul 27, 2021

domenic left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

annevk commented Jul 28, 2021

domenic commented Jul 28, 2021 • edited Loading

dandclark commented Jul 28, 2021

dandclark commented Jun 19, 2020 •

edited by pr-preview bot

Loading

littledan commented Jun 22, 2020 •

edited

Loading

domenic commented Jul 28, 2021 •

edited

Loading