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

Import assertions integration #5883

Closed

Conversation

dandclark
Copy link
Contributor

@dandclark dandclark commented Aug 31, 2020

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 )

dandclark added 27 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.
…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.
…efully unstick CI build, which seems to be stuck.
…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.
…sponse body in the case of a failed type check.
source Outdated Show resolved Hide resolved
@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Nov 16, 2020
@dandclark
Copy link
Contributor Author

dandclark commented Nov 30, 2020

@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 class="note" comments accordingly. Thanks!

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

@annevk annevk Dec 1, 2020

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?

Copy link
Contributor Author

@dandclark dandclark Dec 1, 2020

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.

Copy link
Member

@annevk annevk Dec 1, 2020

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.

Copy link
Contributor Author

@dandclark dandclark Dec 1, 2020

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.

Copy link
Member

@annevk annevk Dec 1, 2020

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.

Copy link
Contributor

@littledan littledan Dec 29, 2020

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?

Copy link
Member

@annevk annevk Mar 10, 2021

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.

Copy link
Contributor Author

@dandclark dandclark Mar 11, 2021

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.

source Outdated Show resolved Hide resolved
pull bot pushed a commit to Alan-love/chromium that referenced this pull request Dec 23, 2020
…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 <daniec@microsoft.com>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839060}
@hiroshige-g
Copy link
Contributor

hiroshige-g commented Dec 29, 2020

visited set should be modified to be a set of (a pair of URL and module type). Otherwise, in the case of

import "./a.js";
import "./a.js" assert { type: "json" };

The whole process of fetching/parsing/etc. of a.js as JSON modules are skipped by visited set (because it already contains an entry for a.js with a different type).

@dandclark
Copy link
Contributor Author

dandclark commented Dec 29, 2020

visited set should be modified to be a set of (a pair of URL and module type). Otherwise, in the case of

import "./a.js";
import "./a.js" assert { type: "json" };

The whole process of fetching/parsing/etc. of a.js as JSON modules are skipped by visited set (because it already contains an entry for a.js with a different type).

Thanks for pointing this out @hiroshige-g, I've added the module type to visited set. It's always just "javascript" in this PR, but corresponding changes in the JSON modules and CSS modules PRs will use the module type from the request.

Base automatically changed from master to main Jan 15, 2021
@dandclark
Copy link
Contributor Author

dandclark commented Mar 10, 2021

@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
Chromium is shipping these soon, and I learned today that an implementation is also underway in Firefox.

Copy link
Member

@annevk annevk left a comment

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.

source Show resolved Hide resolved
@dandclark
Copy link
Contributor Author

dandclark commented Mar 11, 2021

The empty string is not filtered on the ECMAScript side, so type: "" will cause a failure to load the module. I consider this to be correct; I could imagine that there might be some future kind of assertion that might want to distinguish between an empty string value and not being present at all. So I think we should be conservative and not filter these out or special-case them.

type: "javascript" currently fails to load the module, by design. A while back I raised the question of whether we should start allowing it: tc39/proposal-import-assertions#49 (I was asking about type: "js", but it was the same idea). It looks like we didn't come to unanimous agreement on allowing it or not. I'm inclined to again be conservative here and not allow it. It could lead to some confusion especially now that module type is in the module map cache key. If both import "./foo.js" and import "./foo.js" assert { type: "javascript"} both work , do we get two instances of foo.js? Probably not, but I'd prefer not to allow the ambiguity at all since there is no clear use case outside of maybe a slight simplification for some tooling.

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

@annevk
Copy link
Member

annevk commented Mar 11, 2021

Thanks @dandclark! Given that you plan on changing the host hook, the resolution to tc39/proposal-import-assertions#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 type: "json" would be accepted as well. (I'm also okay with other outcomes to be clear, such as accepting type: "javascript", just writing it down to make sure we're all on the same page.)

@dandclark
Copy link
Contributor Author

dandclark commented Mar 12, 2021

@annevk I wrote tc39/proposal-import-assertions#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 import "./foo" assert { type: "js" } or import "./foo" assert { type: "javascript" } work or not by passing or not passing those values to HostGetSupportedExtraModuleScriptTypes. Feedback welcome. 😊

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-assertions#27 (comment) about the web's plans for rejecting unknown types, and I guess we'll see where the discussion goes.

@annevk
Copy link
Member

annevk commented Mar 13, 2021

@dandclark great, even better!

domenic pushed a commit that referenced this pull request Jul 14, 2021
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.
@domenic domenic closed this Jul 29, 2021
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment topic: script
Development

Successfully merging this pull request may close these issues.

None yet

5 participants