Import assertions integration #5883

dandclark · 2020-08-31T18:06:22Z

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.

At least two implementers are interested (and none opposed):
- Firefox: https://mozilla.github.io/standards-positions/#import-attributes
- Chrome: https://groups.google.com/u/1/a/chromium.org/g/blink-dev/c/Xft04J07Oh0/m/s-kUUljbCQAJ
Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/tree/master/html/semantics/scripting-1/the-script-element/import-assertions/
- https://github.com/web-platform-tests/wpt/tree/master/html/semantics/scripting-1/the-script-element/json-module
Implementation bugs are filed:
- Chrome: http://crbug.com/1132413
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1648614, https://bugzilla.mozilla.org/show_bug.cgi?id=1670176
- Safari: …

/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )

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.

…b1078ae

…type.

…oposal spec

…'; 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

…tions

… to 'Import Assertions'

…sponse body in the case of a failed type check.

…attributes

…tch a single module script'.

…ences back to just 'module script'

…attributes-no-json

source

dandclark · 2020-11-30T21:18:07Z

@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

annevk · 2020-12-01T08:58:59Z

source

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


It seems the default implementation is to return the empty list. But above it suggests that passing "type" is a no-op as well?

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.

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.

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.

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.

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

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.

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.

…attributes-no-json

source

…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 · 2020-12-29T04:43:04Z

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

…y evaluate

dandclark · 2020-12-29T23:11:55Z

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.

…pec recommendation is not a sure thing.

…tributes-no-json

dandclark · 2021-03-10T02:20:00Z

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

annevk

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

dandclark · 2021-03-11T02:33:36Z

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-attributes#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 · 2021-03-11T08:06:06Z

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 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 · 2021-03-12T19:15:17Z

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

annevk · 2021-03-13T06:55:47Z

@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 <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} GitOrigin-RevId: b4c95a469497c44c863c4593246783876eae6c18

dandclark added 27 commits June 19, 2020 11:26

Reland JSON modules

914e7d5

Reland JSON modules by reverting a530f6f, resolving merge conflicts, and updating the change with respect to the string changes in a9ef15d.

Plumb through ModuleRequest instead of string for 'fetch an import() …

d0137f5

…module script graph', HostResolveImportedModule, and HostResolveImportedModuleDynamically.

Don't cache a null entry in the module map in the case where the MIME…

b2e7065

… type is valid but doesn't match the requested type.

Allow modulepreload to work for all module types without needing to s…

0cbf597

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

After rebase, fix build break from name change to JSON.Parse in whatwg@…

6932074

…b1078ae

Fix typo in JSON modules example and add note on requiring JSON MIME …

1f83c7e

…type.

Update ModuleRequest definition to reference the import-attributes pr…

a3621dd

…oposal spec

Revert added null check in 'fetch a modulepreload module script graph…

55ebd12

…'; I'll add this in a separate PR.

Reference ParseJSONModule from import-attributes proposal instead of …

af41b28

…defining steps in HTML spec

Cache an entry in the module map if the fetched module was valid but …

a2d4260

…does not match the type specified at the import site.

Extend note to be clear that the module must have no effect until a r…

f79c01f

…equest matches its type and it is evalutated

Wrap <p class=note> in <li> so it's not directly under an <ol>, which…

55c9ed9

… the Travis CI build doesn't like.

Whitespace change: remove double spaces after sentences in notes. Hop…

322808d

…efully unstick CI build, which seems to be stuck.

Merge branch 'upstream-master' into dandclark/import-attributes

43e6fe3

Resolve many merge conflicts in /source.

Reword invocations of 'create a module script with matching type' to …

9f2ae27

…ensure they are synchronous with respect to the calling 'fetch a single module script'

Change 'fetch a single module script' to fail if there are any unreco…

e86dd65

…gnized import attributes, and assert that there is only one of key 'type' (which should be guaranteed by Ecma262)

Instead of passing fetch response parameter to 'create a module scrip…

3318ed3

…t with matching type', retrieve it from the module map within the algorithm.

Add assert that 'create a module script with matching type' is not ru…

3c0dce2

…nning in parallel

Reword caching comment

1c9df5a

Handle rename in TC39 proposal from import attributes to import condi…

3247e99

…tions

Update for the latest rename of the proposal from 'Import Conditions'…

dca376b

… to 'Import Assertions'

Use module type as part of module map cache key instead of caching re…

e3359b5

…sponse body in the case of a failed type check.

Merge remote-tracking branch 'upstream/master' into dandclark/import-…

0857ec8

…attributes

Change 'abort these steps' to 'return' to match recent changes in 'fe…

aafb256

…tch a single module script'.

Remove JSON modules references, switch JavaScript module script refer…

9164acf

…ences back to just 'module script'

dandclark mentioned this pull request Sep 2, 2020

Reland JSON module scripts #5658

Merged

3 tasks

littledan mentioned this pull request Sep 28, 2020

Integration with import-attributes #5640

Closed

Merge remote-tracking branch 'upstream/master' into dandclark/import-…

f53c203

…attributes-no-json

annevk reviewed Nov 16, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Nov 16, 2020

dandclark added 2 commits November 16, 2020 13:09

Move quotes outside of <code> elements.

4ad7d2d

Add HostGetSupportedAssertions integration

614a9f4

annevk reviewed Dec 1, 2020

View reviewed changes

dandclark added 2 commits December 1, 2020 10:27

Fix indentation/spacing. Use short list notation.

2fc3616

Merge remote-tracking branch 'upstream/master' into dandclark/import-…

697a6f1

…attributes-no-json

hiroshige-g reviewed Dec 21, 2020

View reviewed changes

source Outdated Show resolved Hide resolved

dandclark added 2 commits December 29, 2020 15:03

Use two separate scripts in example so that at least one will actuall…

a9eed9b

…y evaluate

Add module type to visited set.

973ec71

Clarify that memory cache mitigating violation of import assertions s…

fdc08f8

…pec recommendation is not a sure thing.

Base automatically changed from master to main January 15, 2021 07:58

Merge remote-tracking branch 'upstream/main' into dandclark/import-at…

30e27fb

…tributes-no-json

annevk reviewed Mar 10, 2021

View reviewed changes

source Show resolved Hide resolved

Add class='XXX' to note about the memory cache

9ec7c7e

dandclark mentioned this pull request Mar 12, 2021

Semantics of unrecognized types tc39/proposal-import-attributes#27

Open

domenic force-pushed the main branch from 0947339 to 1b5099f Compare July 27, 2021 17:07

domenic closed this Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import assertions integration #5883

Import assertions integration #5883

dandclark commented Aug 31, 2020 •

edited by pr-preview bot

Loading

dandclark commented Nov 30, 2020

annevk Dec 1, 2020

dandclark Dec 1, 2020

annevk Dec 1, 2020

dandclark Dec 1, 2020

annevk Dec 1, 2020

littledan Dec 29, 2020

annevk Mar 10, 2021

dandclark Mar 11, 2021

hiroshige-g commented Dec 29, 2020

dandclark commented Dec 29, 2020

dandclark commented Mar 10, 2021

annevk left a comment

dandclark commented Mar 11, 2021

annevk commented Mar 11, 2021

dandclark commented Mar 12, 2021

annevk commented Mar 13, 2021

Import assertions integration #5883

Import assertions integration #5883

Conversation

dandclark commented Aug 31, 2020 • edited by pr-preview bot Loading

dandclark commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroshige-g commented Dec 29, 2020

dandclark commented Dec 29, 2020

dandclark commented Mar 10, 2021

annevk left a comment

Choose a reason for hiding this comment

dandclark commented Mar 11, 2021

annevk commented Mar 11, 2021

dandclark commented Mar 12, 2021

annevk commented Mar 13, 2021

dandclark commented Aug 31, 2020 •

edited by pr-preview bot

Loading