-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Import assertions integration #5883
Import assertions integration #5883
Conversation
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.
…'; I'll add this in a separate PR.
…defining steps in HTML spec
…does not match the type specified at the import site.
…equest matches its type and it is evalutated
…ype, cache the fetch response instead of creating and caching the module script. The module script is created only when there is a type match. This addreses concerns that parsing modules (perhaps even types of modules that don't yet exist) could have side-effects.
… the Travis CI build doesn't like.
…efully unstick CI build, which seems to be stuck.
Resolve many merge conflicts in /source.
…ensure they are synchronous with respect to the calling 'fetch a single module script'
…gnized import attributes, and assert that there is only one of key 'type' (which should be guaranteed by Ecma262)
…t with matching type', retrieve it from the module map within the algorithm.
…nning in parallel
… to 'Import Assertions'
…sponse body in the case of a failed type check.
…tch a single module script'.
…ences back to just 'module script'
…attributes-no-json
@annevk As a last follow up to your review comments I added HostGetSupportedAssertions integration (now that it's reached consensus in TC39) and updated the |
source
Outdated
@@ -91479,6 +91556,17 @@ import "https://example.com/foo/../module2.mjs";</code></pre> | |||
data-x="concept-script-record">record</span>.</p></li> | |||
</ol> | |||
|
|||
<h6><dfn>HostGetSupportedAssertions</dfn>()</h6> |
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.
It seems the default implementation is to return the empty list. But above it suggests that passing "type
" is a no-op as well?
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.
above it suggests that passing "type" is a no-op as well
@annevk Can you clarify which line you're referring to here?
The behavior with this should be that any import assertion with key != "type" is not passed to HTML at all. So we assert in create a module script
that we don't see any import assertion with key != "type". If there is an import assertion with key == "type", though, that will be passed to HTML, who will fail to load the module script if such an import assertion is present (basically to preserve the type
assertion for future use). And then later, values of the type
assertion such as "css" etc will be allowed.
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.
Why are we reserving "type"? If I understand it correctly that is what HTML does differently from other potential hosts and I'm not sure what the reason for it is.
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.
It's not strictly necessary to reserve "type" as part of this PR, since the purpose of this PR is mainly just to set up the plumbing with import assertions in preparation for JSON/CSS modules. However, when landing CSS modules #4898 or other module types then at least at that point we will need to have HostGetSupportedAssertions ask for "type". So my preference is to do it sooner rather than later.
I believe other hosts will follow HTML in rejecting invalid "type" assertion values, e.g. import "./foo" assert { type: "notARealType" }
will be an error in all hosts. For other kinds of assertions, this more conservative approach could be problematic when rolling out new assertions because it would lead to failure on platforms where the new assertion is not supported. But new module types won't work anyway on platforms where they are not yet supported, so this is not adding any new difficulty.
This is in contrast to unknown assertion keys which must now be ignored in all hosts because of HostGetSupportedAssertions.
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.
Oh I see, "type" is the name of the assert field. So if all hosts are doing that, it seems that maybe that should be part of JavaScript, but I guess this works... Wonder if @littledan has thought about that.
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.
@annevk Hmm, I was imagining that we wouldn't specify this, but looking at how HostGetSupportedAssertions works, I guess we could do something analogous, like: hosts would define a list of accepted type values, and then JS would reject values not present. Should we go down this sort of path, to drive more alignment?
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'm not sure if I missed this question due to new years or if we had a discussion about it elsewhere, but looking at it now I think that would be preferable, yes. The less hosts have to coordinate on details the better.
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.
Seems reasonable to me. I'll draft the change to https://tc39.es/proposal-import-assertions/ to see what it will look like, and start an issue in the proposal repo for discussion.
…oduleRequest This is the first in a series of changes that will implement import assertions integration in Blink and tie the assertions into the existing JSON and CSS module prototypes. See the import assertions spec [1], the HTML integration PR [2], and the I2P [3]. This change retrieves the import assertions from V8 in ModuleRequest::ModuleRequests and in the ResolveModuleCallback. The code to unpack them is moved inside blink::ModuleRequest to avoid code duplication across those two places. ModuleRequest is upgraded from a struct to a class now that its constructors are no longer trivial. ModuleRequest is passed to ModuleRecordResolver::Resolve in place of the specifier, where the import assertions will eventually be used alongside the specifier to resolve the module. IsolateHolder now passes "type" as a supported assertion via V8 Isolate::CreateParams so that Blink will start receiving module "type" assertions. Subsequent changes will add the assertions to the module map key and require the correct assertion to be present before a JSON or CSS module can be successfully loaded. [1] https://tc39.es/proposal-import-assertions [2] whatwg/html#5883 [3] https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/Xft04J07Oh0/m/RLPMXVEiAAAJ Bug: 1132413 Change-Id: Id41e9e470d3c4ed1988d8dfa1ef8fa26cd9d23a9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597778 Commit-Queue: Dan Clark <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Reviewed-by: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/master@{#839060}
The whole process of fetching/parsing/etc. of |
Thanks for pointing this out @hiroshige-g, I've added the module type to visited set. It's always just |
…pec recommendation is not a sure thing.
@domenic @annevk @littledan Are we ready to land this PR for import assertions, followed by JSON modules in #5658 (which will shrink after this lands and the import assertions stuff merges out)? Import assertions and JSON modules are both Stage 3 in TC39: https://github.com/tc39/proposals#stage-3 |
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.
There's still one discussion thread open that didn't get a resolution. Perhaps due to me missing the open question. I also found one minor issue and have some further questions about the type
assertion.
Is the empty string type filtered out on the ECMAScript side? So if you do type: ""
we don't get a record? Also, wouldn't the current setup fail with type: "javascript"
? Or is that also filtered out? I didn't see tests for that.
The empty string is not filtered on the ECMAScript side, so
I'm adding tests for the "js" and "javascript" cases with https://chromium-review.googlesource.com/c/chromium/src/+/2751188 (the empty string case is already tested in the empty-type-assertion.js case of https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-script-element/import-assertions/invalid-type-assertion-error.html). |
Thanks @dandclark! Given that you plan on changing the host hook, the resolution to tc39/proposal-import-attributes#49 and what types get accepted by HTML and other hosts will fall out of that, which seems great. So HTML would tell ECMAScript it accepts only "javascript" types and ECMAScript would then basically reject anything with a type annotation. In the future HTML might accept "javascript", "json" and then |
@annevk I wrote tc39/proposal-import-attributes#111 to implement something like this. One thing that differs from your comment is that the way I wrote it, hosts don't have to provide "javascript". JavaScript module support would be assumed (is there ever a reason that this might not be the case?). I also wanted to avoid a design where the host is able to decide whether That said, I'm not sure yet if there will be consensus to land that. There was an old discussion thread on this issue, where there wasn't agreement on whether this behavior should be enforced for all hosts. I left a new comment here tc39/proposal-import-attributes#27 (comment) about the web's plans for rejecting unknown types, and I guess we'll see where the discussion goes. |
@dandclark great, even better! |
This change extends the JavaScript module system integration to include CSS module scripts, in addition to JavaScript module scripts. These allow web developers to load CSS into a component definition in a manner that interacts seamlessly with other module script types. Explainer document: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md Closes WICG/webcomponents#759. Part of #4321. This change includes the integration for the import assertions proposal (https://github.com/tc39/proposal-import-assertions/), thus closing #5640 and closing #5883 by superseding it.
This change extends the JavaScript module system integration to include CSS module scripts, in addition to JavaScript module scripts. These allow web developers to load CSS into a component definition in a manner that interacts seamlessly with other module script types. Explainer document: https://github.com/w3c/webcomponents/blob/gh-pages/proposals/css-modules-v1-explainer.md Closes WICG/webcomponents#759. Part of whatwg#4321. This change includes the integration for the import assertions proposal (https://github.com/tc39/proposal-import-assertions/), thus closing whatwg#5640 and closing whatwg#5883 by superseding it.
…oduleRequest This is the first in a series of changes that will implement import assertions integration in Blink and tie the assertions into the existing JSON and CSS module prototypes. See the import assertions spec [1], the HTML integration PR [2], and the I2P [3]. This change retrieves the import assertions from V8 in ModuleRequest::ModuleRequests and in the ResolveModuleCallback. The code to unpack them is moved inside blink::ModuleRequest to avoid code duplication across those two places. ModuleRequest is upgraded from a struct to a class now that its constructors are no longer trivial. ModuleRequest is passed to ModuleRecordResolver::Resolve in place of the specifier, where the import assertions will eventually be used alongside the specifier to resolve the module. IsolateHolder now passes "type" as a supported assertion via V8 Isolate::CreateParams so that Blink will start receiving module "type" assertions. Subsequent changes will add the assertions to the module map key and require the correct assertion to be present before a JSON or CSS module can be successfully loaded. [1] https://tc39.es/proposal-import-assertions [2] whatwg/html#5883 [3] https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/Xft04J07Oh0/m/RLPMXVEiAAAJ Bug: 1132413 Change-Id: Id41e9e470d3c4ed1988d8dfa1ef8fa26cd9d23a9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597778 Commit-Queue: Dan Clark <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Reviewed-by: Jochen Eisinger <[email protected]> Reviewed-by: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/master@{#839060} GitOrigin-RevId: b4c95a469497c44c863c4593246783876eae6c18
This change integrates import attributes as proposed at https://github.com/tc39/proposal-import-attributes. This is a subset of the change at #5658, with the JSON modules implementation removed. Since JSON modules was split off into a separate proposal in TC39, this PR anticipates the possibility that import assertions are standardized prior to JSON modules. In that event, this PR can be landed to unblock other module types such as CSS modules without waiting for JSON modules.
Until other module types are built on top of this PR, it's almost a no-op; the presence of any module assertions will cause a module load to fail. To reflect the discussion at #5658 and #5640, this PR includes module type in the module map cache key, but until other module types come along this will have no effect because it will always be undefined.
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )