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 assertion type: 'javascript' #7342

Closed
GeoffreyBooth opened this issue Nov 18, 2021 · 13 comments · Fixed by #7350
Closed

Import assertion type: 'javascript' #7342

GeoffreyBooth opened this issue Nov 18, 2021 · 13 comments · Fixed by #7350

Comments

@GeoffreyBooth
Copy link

Hi, I’m implementing support for import assertions in Node.js. I’m trying to make Node’s support for the feature align with the HTML spec for the module types that both Node and browsers support (JavaScript, JSON, potentially WebAssembly). I’m also trying to match some of the requirements of the HTML spec, such as including the assertion type in the module cache key, so that Node behaves similarly to browsers with regard to race conditions and module identity.

While reading the spec, I saw language such as this:

If moduleRequest.[[Assertions]] has a Record entry such that entry.[[Key]] is "type", then let module type be entry.[[Value]]. Otherwise let module type be "javascript".

And this:

If module type is not "javascript", "css", or "json", then return false.

And I assumed that this meant that the spec permitted javascript as a module type. In other words, that import './file.js' assert { type: 'javascript' } should be allowed and treated equivalently as import './file.js', and both imports would map to the same entry in the module map, because they have the same type. I was so certain of this reading of the spec that I opened tc39/proposal-import-attributes#114 to suggest upstreaming this “treat the lack of a declared type as if type: 'javascript' was specified” behavior into the import assertions proposal. I opened nodejs/node#40790 and an accompanying PR to propose adding this behavior to Node, and when someone there pointed out that Chrome errors on type: 'javascript' I opened https://bugs.chromium.org/p/chromium/issues/detail?id=1270635 to report what I assumed was a bug in Chrome.

But . . . am I misreading the spec? Is there some intent to disallow an explicit assert { type: 'javascript' }? @dandclark seemed to think so when he pushed https://chromium-review.googlesource.com/c/chromium/src/+/2751188 to add tests to Chromium that it should error on an explicit type: 'javascript'. I assumed that back when those tests were written perhaps the spec was different, and it’s since expanded the scope of type: 'javascript'; but am I wrong about this?

At the very least, the spec could be clearer one way or the other about whether an explicit assert { type: 'javascript' } for a JavaScript module is expected to succeed or fail. Everything I read in the spec text led me to assume that it should succeed (because why should it fail, if it’s implied by a missing type) so if it’s supposed to error, I would be very curious as to why that is and what that means for the “default” or “implied” module type.

@annevk
Copy link
Member

annevk commented Nov 18, 2021

I agree with your reading of the specification that type: "javascript" ought to work. https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-script-element/import-assertions/invalid-type-assertion-error.html contradicts this though.

@dandclark @domenic can you take a look?

@guybedford
Copy link
Contributor

Looking more closely at the import assertions specification, it actually describes in the host integration notes treating JavaScript as "type": undefined (https://tc39.es/proposal-import-assertions/#sec-host-integration) so it seems this might be a spec conflict that needs to be worked out as well.

@dandclark
Copy link
Contributor

I think you're both correct, the spec as written allows type: "javascript". My intent when writing it was that type: "javascript" should not work. At one point I changed the spec draft for readability reasons so that instead of passing around a null module type for JS, we'd pass around the "javascript" string instead. That's probably when I introduced the issue. Apologies for the confusion here.

I'd intended type: "javascript" not to work because I'd been trying to see if we could have the import assertions spec in TC39 standardize behavior about "unknown" types, and about type: "javascript", but those efforts stalled out, e.g. here. Lacking a conclusive resolution from TC39, I intended to go with the more restrictive option (disallowing type: "javascript"), expecting that it would be less of a compat risk to allow type: "javascript" later if we chose that route, than to go the other way around.

I think it's unlikely that this behavior will ever be specified either way in TC39, but I'm open to considering either direction here for HTML and React. We could either leave the HTML spec as-is and change the Chromium implementation, or update the spec so that it explicitly bans type: "javascript".

As I see it, the advantage of allowing type: "javascript" would mainly be for consistency with the other types. Developers might expect it to work, and allowing it might make code generation slightly simpler in some cases.

A minor disadvantage of allowing it is simply that there would be more than one way to do the exact same thing, which is maybe non-ideal.
It could also be confusing that import foo from "./foo.js" and import foo from "./foo.js" assert { type: "javascript"} will both refer to the same module instance in the module map.
Another complication is that there's some ambiguity about what the string should be -- "javascript", "js", simply "script"? Will we also need "webassembly" and/or "wasm"?

Overall I prefer to sidestep those issues by banning type: "javascript", but don't feel strongly about it. Thoughts?

Regarding the https://tc39.es/proposal-import-assertions/#sec-host-integration section, that's non-normative and was written well before the HTML spec integration in order to paint a picture of that would eventually look like. Now that the HTML spec integration has been done, I'm thinking that maybe we could just delete that section, since the actual HTML spec is now the source of truth.

@GeoffreyBooth
Copy link
Author

So I’ve been refereeing (and somewhat part of) a debate in nodejs/node#40790 and tc39/proposal-import-attributes#114 regarding how to handle this. The part I feel strongly about is what I titled tc39/proposal-import-attributes#114: that every “module format” (like JavaScript, JSON, CSS and Wasm) have exactly one valid type. Doing so makes the implementation much simpler, and avoids race conditions and a lot of associated complexity. This constraint is already present in the HTML spec by virtue of unspecified modules getting 'javascript' and JSON requiring 'json' and CSS requiring 'css'; it’s unclear whether the HTML spec will continue this pattern in the future, but I encourage you to do so. It’s already mentioned as an aside here:

Additionally, as long as all module types are mutually exclusive, the module type check in fetch a single module script will fail for at least one of the imports, so at most one module evaluation will occur.

And I think moving this into a more formal/explicit part of the spec would be a good idea.

Beyond that, though, my goal is to try to make code written for Node interoperable with browsers as much as possible. So I don’t want Node to require type: 'webassembly' while browsers to require type: 'wasm', or one of us to require no type, or something like that. It’s unfortunate that this kind of standardization isn’t happening in the TC39 spec, but Node can choose to follow the HTML spec’s lead on this and that covers the compatibility cases we care about.

The place where this has become an issue is with regard to Wasm. @guybedford and others in the Wasm community would strongly prefer Wasm to be importable without a mandatory type assertion; so import foo from './foo.wasm', not import foo from './foo.wasm' assert { type: 'webassembly' } or type: 'wasm'. This would make it easier to swap out a JavaScript module for a Wasm one or vice versa, since the importing site wouldn’t necessarily need to update as a result of that change (assuming the path remains the same, e.g. import foo from '/foo' or import bar from 'bar'). With the HTML spec as currently written, if Wasm is allowed to be imported without a type assertion, it would be given the module type 'javascript' when it’s loaded into the module map. Even worse, import './foo.wasm' assert { type: 'javascript' } would be expected to work, which may confuse users.

What @guybedford and I think might be the cleanest solution is to simply rename 'javascript' in the HTML spec. It could get a name describing its capability rather than its format, since the purpose of type assertions is to restrict the capability of the imported module (ensuring that JSON can’t execute, for example). We were thinking that executable would be a good replacement name. Fortunately no JavaScript runtime currently supports type: 'javascript', so it wouldn’t be a breaking change to update the HTML spec. (And we could close https://bugs.chromium.org/p/chromium/issues/detail?id=1270635 if there’s an intent to change this part of the HTML spec.)

If you agree with type: 'executable' (or some other string that means the same thing), all of the following would become allowable:

import './module.js';
import './module.js' assert { type: 'executable' };
import './module.wasm';
import './module.wasm' assert { type: 'executable' };

These would correspond to two entries in the module map: module.js as type: 'executable', and module.wasm as type: 'executable'. In other words, the lack of an explicit type would be interpreted as if assert { type: 'executable' } had been written there.

I think it’s good to allow the implicit type to be specified, so that users can write code like import(url, { type: dynamicType }) where dynamicType could be a variable with values like 'json' or 'executable'. It also just makes logical sense that a permitted type should be allowed to be specified.

Beyond potentially renaming type: 'javascript', if the HTML spec could further declare that WebAssembly modules will share this implicit default type with JavaScript, that would help move the ball forward in standardizing import of Wasm. What do you think?

@ljharb
Copy link
Contributor

ljharb commented Nov 19, 2021

have exactly one valid type

This is not how this feature was intended or designed. A module format may have 0, 1, or N valid types.

@annevk
Copy link
Member

annevk commented Nov 19, 2021

@dandclark I think we should ban it for now. We can revisit when we're tackling Wasm or TC39 decides it's useful to have a name for the default.

@Ms2ger
Copy link
Member

Ms2ger commented Nov 19, 2021

I agree we should ban it - it makes more sense for the default to allow both JS and Wasm. Looks like Chromium already implements the ban, indeed.

@domenic
Copy link
Member

domenic commented Nov 19, 2021

if the HTML spec could further declare that WebAssembly modules will share this implicit default type with JavaScript, that would help move the ball forward in standardizing import of Wasm. What do you think?

I don't think there's any work on wasm modules on the web right now, so we can't make that sort of decision for you at this point. Someone else can correct me if I'm wrong.

@GeoffreyBooth
Copy link
Author

I don’t think there’s any work on wasm modules on the web right now, so we can’t make that sort of decision for you at this point.

There’s some discussion in WebAssembly/esm-integration#42.

Essentially though, if “lack of type” is assigned to 'javascript', the way the spec is written now, that implies that WebAssembly must have a type, because if Wasm were to be allowed without a type then it would also be mapped to 'javascript'. This is fine from a technical perspective, but for usability it could be confusing, hence the suggestion to rename 'javascript' to something more generic. So basically the spec as written now does make the decision that if/when Wasm is supported via import, it will presumably require a mandatory type, just as JSON does.

Whether the implicit type is renamed to 'executable', some other string, or undefined doesn’t make much difference; any option would solve this issue. I would prefer to pick a string rather than undefined so that every format has a valid type it would map to, which makes more sense to me as a user, but if you’d prefer to go with undefined then that would work.

@domenic
Copy link
Member

domenic commented Nov 19, 2021

I think we should keep the implicit type as "javascript", but disallow it from appearing explicitly. Then we can change it later in the spec if we decide to share it with webassembly, since such a change would be only in the spec and not observable.

But for now the string in the spec should (accurately) reflect the fact that such modules, in the spec, are JavaScript modules.

@dandclark
Copy link
Contributor

I think we should keep the implicit type as "javascript", but disallow it from appearing explicitly. Then we can change it later in the spec if we decide to share it with webassembly, since such a change would be only in the spec and not observable.

But for now the string in the spec should (accurately) reflect the fact that such modules, in the spec, are JavaScript modules.

Agreed. I put together a PR for this: #7350.

@GeoffreyBooth
Copy link
Author

But for now the string in the spec should (accurately) reflect the fact that such modules, in the spec, are JavaScript modules.

Do you mind sharing the thought process behind how this decision was made in the first place, and how you feel about it now? As in, why have a string 'javascript' in the module map at all, as opposed to say the undefined value suggested by the host integration example?

I’m not trying to imply that the decision was wrong; I just want to understand it, to get a sense of how this team feels about import assertions generally so that I can try to predict the future of the HTML spec with regard to this feature. For example, the fact that 'javascript' was chosen as the default seems to me to strongly imply that the HTML spec would require all non-JavaScript module types to have a distinct type, like JSON and CSS do now; so Wasm would require a type: 'webassembly' or type: 'wasm', whichever is decided upon. But it sounds like that isn’t necessarily decided, so now I’m curious how 'javascript' came to be and what I can infer from that choice.

@annevk
Copy link
Member

annevk commented Nov 23, 2021

The HTML standard defines these concepts:

  • JavaScript module scripts
  • JSON module scripts
  • CSS module scripts

The identifiers reflect these concepts. If WebAssembly module scripts become a thing and there's a collaborative decision that they are indistinguishable from JavaScript module scripts, I suspect we'd pick a new name that represents both. In other words, I wouldn't try to read too much into anything. Things are pretty flexible as long as they are not exposed to the web.

domenic pushed a commit that referenced this issue Nov 23, 2021
This was the original intent of the import assertions integration, but type: "javascript" was unintentionally allowed to work due to a bug involving the spec's internal use of the "javascript" module type for JavaScript module scripts.

Fixes #7342.
dandclark added a commit to dandclark/html that referenced this issue Dec 4, 2021
This was the original intent of the import assertions integration, but type: "javascript" was unintentionally allowed to work due to a bug involving the spec's internal use of the "javascript" module type for JavaScript module scripts.

Fixes whatwg#7342.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
This was the original intent of the import assertions integration, but type: "javascript" was unintentionally allowed to work due to a bug involving the spec's internal use of the "javascript" module type for JavaScript module scripts.

Fixes whatwg#7342.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

7 participants