Invalid attributes keys in static imports should be a resolution/loading error and not a parsing error

[nicolo-ribaudo]

import "x" with { unknownAttr: "y" }

Should parse successfully, and fail trying to load the imported module.

This is not currently observable within ECMA-262, but it affects how errors are reported for code that contains an actual syntax error:

import "x" with { unknownAttr: "y" }

a b c

Reporting the parsing error at the attribute rather than at the missing semicolon after a is very awkward.

[nicolo-ribaudo]

This has been fixed.

[nicolo-ribaudo]

I'm reopening this because I noticed that this change caused an assertion in HTML to be invalid (step 9 of https://html.spec.whatwg.org/#creating-a-javascript-module-script).

Right now, ignoring that assertion, when loading a module this is what happens:

1. (HTML) Fetch the module source code (this might throw)
2. (ECMA-262) Parse the module (this might throw)
3. (HTML) For each import of the module,
   1. (HTML) Resolve the URL (this might throw) — step 10.1 of https://html.spec.whatwg.org/#creating-a-javascript-module-script
   2. (HTML) Check that it has a valid type attribute, if any (this might throw)
4. (ECMA-262) For each imported module,
   1. (ECMA-262) Check that it does not have any unknown attribute (this might throw)
   2. (HTML) Resolve the URL — step 8 of https://html.spec.whatwg.org/#hostloadimportedmodule
   3. Re-run all these steps for the new module

We have two options:

1. Duplicate step 4.1 in 3, similarly to how 4.2 is already duplicated to report resolution errors early
2. Just remove the assertion, which doesn't add any value to the "create a JavaScript module script" algorithm. Note that the assertion in https://html.spec.whatwg.org/#fetch-a-single-imported-module-script would still hold, because it happens in 4.3.

I'm implementing this in chromium, and the complexity of doing 1 vs 2 is the same. I have a slight preference for duplicating the check, to align it with modules URL resolution. For dynamic import, currently the check for invalid attribute keys happens before trying to resolve the URL (because it happens in ECMA-262 before calling in HTML).

cc @domenic

[nicolo-ribaudo]

Doing (2) also has the benefit of aligning static and dynamic imports.

Given this code:

await Promise.all([
    import("https://example.com/x", { with: { type: "foo" } }).catch(e => e.message),
    import("x", { with: { type: "foo" } }).catch(e => e.message),
    import("https://example.com/x", { with: { invalid: "foo" } }).catch(e => e.message),
    import("x", { with: { invalid: "foo" } }).catch(e => e.message),
    import('data:text/javascript, import "https://example.com/x" with { type: "foo" }').catch(e => e.message),
    import('data:text/javascript, import "x" with { type: "foo" }').catch(e => e.message),
    import('data:text/javascript, import "https://example.com/x" with { invalid: "foo" }').catch(e => e.message),
    import('data:text/javascript, import "x" with { invalid: "foo" }').catch(e => e.message),
]);

with (1) it would print

[
  "\"foo\" is not a valid module type.",
  "Failed to resolve module specifier 'x'",
  "Invalid attribute key \"invalid\".",
  "Invalid attribute key \"invalid\".",
  "\"foo\" is not a valid module type.",
  "Failed to resolve module specifier \"x\".",
  "Invalid attribute key \"invalid\".",
  "Failed to resolve module specifier \"x\".",
]

while with (2) it would print

[
  "\"foo\" is not a valid module type.",
  "Failed to resolve module specifier 'x'",
  "Invalid attribute key \"invalid\".",
  "Invalid attribute key \"invalid\".",
  "\"foo\" is not a valid module type.",
  "Failed to resolve module specifier \"x\".",
  "Invalid attribute key \"invalid\".",
  "Invalid attribute key \"invalid\"."
]

[domenic]

I don't think I have strong feelings here but all else being equal, aligning dynamic and static imports seems nice.

The assert in step 9 does seem pretty inconsequential to me so removing it from HTML doesn't seem like a big deal.

[nicolo-ribaudo]

Fixed in whatwg/html#9599