-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
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.
34933d4
to
6932074
Compare
There was a problem hiding this comment.
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
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.
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).
There was a problem hiding this comment.
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.
@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, |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only editorial issues!
There was a problem hiding this comment.
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. |
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 )