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

Reland JSON module scripts #5658

Merged
merged 71 commits into from Jul 29, 2021
Merged

Conversation

dandclark
Copy link
Contributor

@dandclark dandclark commented Jun 19, 2020

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.


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

@domenic
Copy link
Member

domenic commented Jun 19, 2020

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.

dandclark added 6 commits Jun 19, 2020
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.
@dandclark dandclark force-pushed the dandclark/import-attributes branch from 34933d4 to 6932074 Compare Jun 19, 2020
Copy link
Contributor

@littledan littledan left a comment

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
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
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.
-->
Copy link
Contributor

@littledan littledan Jun 22, 2020

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

Copy link
Contributor Author

@dandclark dandclark Jul 27, 2021

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
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@domenic
Copy link
Member

domenic commented Jun 22, 2020

@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
Copy link
Contributor

littledan commented Jun 22, 2020

@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
Copy link
Member

domenic commented Jun 22, 2020

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
Copy link
Contributor

littledan commented Jun 22, 2020

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
Copy link
Member

domenic commented Jun 22, 2020

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.

@littledan
Copy link
Contributor

littledan commented Jun 22, 2020

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
Copy link
Member

domenic commented Jun 22, 2020

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

@littledan
Copy link
Contributor

littledan commented Jun 22, 2020

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
Copy link
Member

domenic commented Jun 22, 2020

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

@littledan
Copy link
Contributor

littledan commented Jun 22, 2020

@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
Copy link
Member

domenic commented Jun 22, 2020

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

@littledan
Copy link
Contributor

littledan commented Jun 22, 2020

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
Copy link
Member

domenic commented Jun 22, 2020

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
Copy link

matthewp commented Jun 23, 2020

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
Copy link

matthewp commented Jun 23, 2020

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 dandclark mentioned this pull request Jun 25, 2021
3 tasks
@dandclark dandclark changed the title Add import assertions integration and reland JSON modules Reland JSON modules Jul 27, 2021
@dandclark dandclark marked this pull request as ready for review Jul 27, 2021
@dandclark
Copy link
Contributor Author

dandclark commented Jul 27, 2021

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 dandclark changed the title Reland JSON modules Reland JSON module scripts Jul 27, 2021
Copy link
Member

@domenic domenic left a comment

LGTM, only editorial issues!

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
Copy link
Member

@domenic domenic left a comment

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

@annevk
Copy link
Member

annevk commented Jul 28, 2021

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
Copy link
Member

domenic commented Jul 28, 2021

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
Copy link
Contributor Author

dandclark commented Jul 28, 2021

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

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
@domenic domenic added topic: script addition/proposal New features or enhancements labels Jul 29, 2021
@domenic domenic merged commit d554d6d into whatwg:main Jul 29, 2021
2 checks passed
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

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}
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

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}
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

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

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
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: script
Development

Successfully merging this pull request may close these issues.

None yet

7 participants