Reland JSON module scripts #5658
Conversation
|
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. |
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
force-pushed
the
dandclark/import-attributes
branch
from
34933d4
to
6932074
Compare
There was a problem hiding this 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
| 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. | ||
| --> |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
@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 For example, we shouldn't allow changing |
|
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, |
dandclark
changed the title
|
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! |
|
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. |
|
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 |
|
I'm adding tests for the UTF-16 BOM cases here: https://chromium-review.googlesource.com/c/chromium/src/+/3059241 I'm removing |
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}
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}
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
…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
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 )