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

Allow assertions to affect module loading, change keyword to with #131

Merged
merged 3 commits into from Mar 14, 2023

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 2, 2023

This PR is based on the outcome of the calls of the modules group in the past weeks.

PREVIEW: https://nicolo-ribaudo.github.io/proposal-import-assertions/

There are three main parts:

  • Allow assertions to affect module loading
    This is the main goal we have based on the HTML feedback. Attributes can now be part of the cache key (previously they could but it was discouraged) and they can be used to affect how a module is fetched/interpreted.

  • Throw on unsupported assertions
    To maximize portability, the previous version of the import assertions proposal ignored any assertion not supported by the host. That was the best choice, because unless someone was explicitly expecting the assertion to throw the could would have continued working across environments.
    Now that import attributes can change the result of importing a module, ignoring them may completely change the runtime behavior of the module (e.g. think about type: "css-inject-global" vs type: "css-module") and it's better to error early.

  • Change the keyword from assert to with
    Import attributes are not anymore about asserting properties of a module (or at least, they are not according to the commonly understood definition of assert in programming languages), so this proposal is changing the keyword from assert to with.

    We are aware that there are shipping unflagged implementations using the assert keyword, and that changing keyword can be very difficult if not impossible. Our plan is to encourage a gradual long migration with assert as an alias to with, and if this migration happens successfully we'll be able to remove the assert keyword at some point in the future.

When discussing about this proposal, we are now referring to it as "attributes" instead of "assertions". This PR doesn't change that yet, and instead only focuses on semantics (since it's just a communication/editorial change).

We also discussed about suggesting tools to not use import assertions unless they are explicitly prefixed and marked as "this only works in my tool" so that users don't expect them to be portable (for example, _webpackLoader: "css"). I'm not sure about where/how to write that.

cc @guybedford @jridgewell @littledan

status: proposal
stage: 2
location: https://github.com/tc39/proposal-import-assertions
copyright: false
contributors: Sven Sauleau, Myles Borins, Daniel Ehrenberg, Daniel Clark
</pre>

<script>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These <script>/<style> tags are needed to get this rendering:

1. For each String _key_ of _keys_,
1. Let _value_ be Completion(Get(_assertionsObj_, _key_)).
1. IfAbruptRejectPromise(_value_, _promiseCapability_).
1. If Type(_value_) is not String, then
1. Perform ! Call(_promiseCapability_.[[Reject]], *undefined*, &laquo; a newly created *TypeError* object &raquo;).
1. Return _promiseCapability_.[[Promise]].
1. If _supportedAssertions_ contains _key_, then
1. Append the ImportAssertion Record { [[Key]]: _key_, [[Value]]: _value_ } to _assertions_.
1. Append the ImportAssertion Record { [[Key]]: _key_, [[Value]]: _value_ } to _assertions_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are they still called assertion records?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To minimize the diff and make it easier to see what are the changes, I kept the "assertions" name everywhere (otherwise the diff would have affected a much higher portion of the file). Changing it to "attributes" is just an editorial change, and I plan to do it as soon as we have an agreement on semantics and merged the semantic updates.

@ljharb
Copy link
Member

ljharb commented Mar 2, 2023

When you say "Throw on unsupported assertions", obviously this is what import() would have to do - but are unsupported attributes an early error in static import?

@GeoffreyBooth
Copy link

If the runtime throws on unsupported attributes, how would new attribute keys ever be able to be added? I would think that for browsers this would mean that whatever attribute keys are supported when this syntax initially ships would be the only keys ever supported, as no one would be able to use a new key without worrying that some older browser that doesn’t support the key would throw on encountering it. (Or there would be a lag time of years for each new attribute key added, so that authors are confident that older browsers are disused enough that the new attribute is supported by all the browsers they care about.) Likewise with libraries written for Node, where library authors would have to worry about older versions of Node throwing on an unsupported key.

I’d also like to hear from the browser folks how the HTML spec plans to support various attribute keys, such as type and whatever else comes later. Currently type is part of the module map key; would every other supported key also become part of the cache key? When implementing import assertions for Node, I found that I needed to follow how the HTML spec handled module map cache keys in order to provide consistent behavior with how Node and browsers each handle dynamic import() race conditions; I raised this in #114. This is also something that could or arguably should be standardized here, rather than in the HTML spec that server-side runtimes would also need follow in order to provide consistent behavior with browsers.

@justinfagnani
Copy link

how would new attribute keys ever be able to be added?

The same way as any new syntax, right? Compilers will have to downlevel new keys and new values for older browsers.

@ljharb
Copy link
Member

ljharb commented Mar 2, 2023

This is syntax, and any new syntax can't generally be added in a nonbreaking way. Things that aren't supported shouldn't parse.

@GeoffreyBooth
Copy link

The same way as any new syntax, right?

The previous syntax, and as far as I'm aware this one too, treats the keys in the assertion “object” as user-defined strings; they're not keywords. As such I'd expect to be able to add other keys. I thought that was the point of using the object form.

@justinfagnani
Copy link

justinfagnani commented Mar 3, 2023

Attribute values seem to be very much like import specifiers. They look like strings, but they are values that different hosts do and do not support, and if a value isn't supported there's an error. Toolchains and import maps deal with translating values to supported forms.

edit: and in the case of import() they really are strings, and tool support for transformations is just more complicated - they don't support dynamic values, etc.

@GeoffreyBooth
Copy link

Attribute values seem to be very much like import specifiers. They look like strings,

I’m asking about keys, not values. From https://github.com/tc39/proposal-import-assertions/#proposed-syntax:

Here, a key-value syntax is used, with the key type used as an example indicating the module type. Such key-value syntax can be used in various different contexts.

And in this PR, in https://github.com/tc39/proposal-import-assertions/pull/131/files#diff-7d681727fcf47dc0b9a7512a470fb0da63276c625891a5cc232d725bd12912fdR139:

1. Append the ImportAssertion Record { [[Key]]: _key_, [[Value]]: _value_ } to _assertions_.

If the plan is for runtimes to throw on any unrecognized keys (and values?), I guess that’s an option, but it’s a significant change from the previous Stage 3 proposal. It would seem to significantly slow the introduction of new keys and/or values. Ideally there would be some way to gracefully fall back. Maybe that’s just dynamic import() and try/catch? Presumably the error is a runtime one and not a parse error?

I would also appreciate an answer about whether type will continue to be part of the module map for browsers (and by implication, everyone) and if that means we would be following that pattern for any other new keys that follow.

@ljharb
Copy link
Member

ljharb commented Mar 3, 2023

@GeoffreyBooth imo the point of the object form is that it's expandable in the future without additional grammar changes - but only TC39 would be adding new ones, not individual hosts or tools.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Mar 3, 2023

@ljharb
When you say "Throw on unsupported assertions", obviously this is what import() would have to do - but are unsupported attributes an early error in static import?

Yes, it's an early SyntaxError, defined at https://github.com/nicolo-ribaudo/proposal-import-assertions/blob/f5e05308608689068c48066bda6861d94f0ce34e/spec.emu#L682. For dynamic import, it's a TypeError, defined at https://github.com/nicolo-ribaudo/proposal-import-assertions/blob/f5e05308608689068c48066bda6861d94f0ce34e/spec.emu#L141.

@GeoffreyBooth
If the runtime throws on unsupported attributes, how would new attribute keys ever be able to be added? I would think that for browsers this would mean that whatever attribute keys are supported when this syntax initially ships would be the only keys ever supported, as no one would be able to use a new key without worrying that some older browser that doesn’t support the key would throw on encountering it. (Or there would be a lag time of years for each new attribute key added, so that authors are confident that older browsers are disused enough that the new attribute is supported by all the browsers they care about.) Likewise with libraries written for Node, where library authors would have to worry about older versions of Node throwing on an unsupported key.

We have thought about this for a while, and there are three options for unknown keys:

  • ignore (as in the current version of the proposal)
  • pass everything to the host, and let the host decide if it should throw or ignore
  • throw

Ignoring them was certainly the best choice with the current version of the proposal, where these modifiers are really just assertions: ignoring a (passing) assertion doesn't change the behavior of the code, so unknown assertions would have also worked in older engines.

However, now that these modifiers can affect how a module is evaluated, it's not usually a valid fallback mechanism anymore. Consider a few hypothetical examples:

  • import "mod" with { resolutionMode: "importmap" }, which would make Node.js resolve "mod" from an external import_map.json file (like HTML and Deno) instead of using the default node_modules-based resolution algorithm
  • import "./image.png" with { type: "image", as: "arraybuffer" }, import "./image.png" with { type: "image", as: "dom" }, import "./image.png" with { type: "image", as: "canvas" }, which would allow browsers to import images respectively as ArrayBuffer, HTMLImageElement, and CanvasRenderingContext2D objects (with one of them being the default, assuming that type gets implemented before as).
  • import styles from "./main.css" with { type: "css", layer: "utilities" }, that would allow importing a specific CSS layer, similar to @import "./main.css" layer(utilities).

All these example attributes cannot be simply ignored:

  • Ignoring resolutionMode would likely throw at module resolution time anyway, because the module cannot be found using the default strategy (unless there is a duplicated mod module in node_modules, in which case it risks loading an unexpected module).
  • Ignoring as would give you a different object than what you expect, likely throwing at execution time when you try to use it.
  • Ignoring layer for CSS would still give you a working CSSStyleSheet object, except that it would have different CSS rules than what you expect and would cause the resulting styles to be all broken.

On the other hand, there might also be attributes that could safely be ignored. However, I couldn't find examples compelling enough to justify not throwing given all the cases where throwing would be better.

@GeoffreyBooth
I’d also like to hear from the browser folks how the HTML spec plans to support various attribute keys, such as type and whatever else comes later. Currently type is part of the module map key; would every other supported key also become part of the cache key?

Yes, type will continue to be part of the module map for browsers. Part of the motivation for the changes to this proposal is that the current version recommends to not use assertions as part of the module map keys, but HTML (and by extension, everyone who is trying to match HTML) ignores that recommendation.

For other keys, it depends. For example, a possible resolutionMode key would need to be part of the module map key (import "mod" with { resolutionMode: "node_modules" } and import "mod" with { resolutionMode: "importmap" } may resolve to different modules), while a possible noHTTPImports key doesn't (once the first between import "mod" and import "mod" with { noHTTPImports: "true" } resolves, they would always both resolve to the same module).

This proposal leaves the choice to hosts. The way it's specified, from ECMA-262's point of view all the assertions are part of the module map key, but hosts are free to have the same module corresponding to different keys (same as how "./dep" and "../dir/dep"` can be mapped to the same module). Implementation-wise, I would expect the module map to only be actually keyed on the attributes that matter.

@ljharb
imo the point of the object form is that it's expandable in the future without additional grammar changes - but only TC39 would be adding new ones, not individual hosts or tools.

Note that this PR doesn't change this yet, and it keeps the original "host gives to ECMA-262 the list of attributes it supports" strategy. There are some discussions about how to potentially restrict this, while still acknowledging that:

  • some hosts or tools would want to provide their own modifiers, similarly to how they have their own DSLs in specifiers
  • portability is a shared goal, and hosts would very likely converge on a shared set of supported attributes (for example, I would expect the various WinterWG runtimes and HTML to all support the same attributes, except for client-specific stuff such as maybe CSS modules)

@gibson042
Copy link

@GeoffreyBooth
If the runtime throws on unsupported attributes, how would new attribute keys ever be able to be added? I would think that for browsers this would mean that whatever attribute keys are supported when this syntax initially ships would be the only keys ever supported, as no one would be able to use a new key without worrying that some older browser that doesn’t support the key would throw on encountering it. (Or there would be a lag time of years for each new attribute key added, so that authors are confident that older browsers are disused enough that the new attribute is supported by all the browsers they care about.) Likewise with libraries written for Node, where library authors would have to worry about older versions of Node throwing on an unsupported key.

We have thought about this for a while, and there are three options for unknown keys:

  • ignore (as in the current version of the proposal)
  • pass everything to the host, and let the host decide if it should throw or ignore
  • throw

Ignoring them was certainly the best choice with the current version of the proposal, where these modifiers are really just assertions: ignoring a (passing) assertion doesn't change the behavior of the code, so unknown assertions would have also worked in older engines.

However, now that these modifiers can affect how a module is evaluated, it's not usually a valid fallback mechanism anymore.

I think this is glossing over an upgrade concern that was deferrable on the basis of unknown attributes being ignored (cf. #21 (comment) and #56 (comment) ). Will authors be required to split their source files in order to leverage new attributes in static imports without breaking old (or even not-so-old-but-not-bleeding-edge) engines? I guess so, but that implies an outer mechanism like import maps (and tooling to leverage it, especially regarding deep imports) that isn't available to an arbitrary ECMAScript implementation.

Consider a few hypothetical examples:

  • import "mod" with { resolutionMode: "importmap" }, which would make Node.js resolve "mod" from an external import_map.json file (like HTML and Deno) instead of using the default node_modules-based resolution algorithm
  • import "./image.png" with { type: "image", as: "arraybuffer" }, import "./image.png" with { type: "image", as: "dom" }, import "./image.png" with { type: "image", as: "canvas" }, which would allow browsers to import images respectively as ArrayBuffer, HTMLImageElement, and CanvasRenderingContext2D objects (with one of them being the default, assuming that type gets implemented before as).
  • import styles from "./main.css" with { type: "css", layer: "utilities" }, that would allow importing a specific CSS layer, similar to @import "./main.css" layer(utilities).

Thank you for the examples. I think the middle one might be better spelled with distinct type values like "image-arraybuffer" vs. "image-dom" vs. "image-canvas" since the resulting imports would have different shapes, and I'm not sure that old engines refusing to load the third example (rather than ignoring layer and executing later feature-detection-dependent fixup) is a good thing, but the first example does seem salient.

On the other hand, there might also be attributes that could safely be ignored. However, I couldn't find examples compelling enough to justify not throwing given all the cases where throwing would be better.

What about building in an escape hatch? If the import itself has a means of expressing attribute-level ignorable/required status (subject to host approval, e.g. type is never ignorable), then we get behavior aligned with anticipated predominant use cases but avoid precluding future alternatives.

There's recent if somewhat distant precedent for a similar concept affecting RFC 3339 timestamp suffixes, in which a leading ! opts in to criticality such that 2022-07-08T00:14:07+01:00[knort=blargel] may be accepted while 2022-07-08T00:14:07+01:00[!knort=blargel] is rejected.

@justinfagnani
Copy link

@gibson042

I think the middle one might be better spelled with distinct type values like "image-arraybuffer" vs. "image-dom" vs. "image-canvas" since the resulting imports would have different shapes

This point might need clarification with the update. With assertions my understanding is that type did not change the shape of the imported module - that was determined solely by the mime-type in browsers and extension in Node - it only asserted that the file type matched the import type.

That changes a bit if HTML uses type to send a header, but it doesn't change the fact that type: 'css' always expected the file to be CSS, or that type: 'image' would always expect the file to be an image, regardless of whatever form that was transformed into.

ie, for CSS you might have, {type: 'css', as: 'text'} to say that you require that you load an actual CSS file, but you don't want it parsed into a stylesheet.

@nicolo-ribaudo
Copy link
Member Author

@gibson042
I think this is glossing over an upgrade concern that was deferrable on the basis of unknown attributes being ignored (cf. #21 (comment) and #56 (comment) ). Will authors be required to split their source files in order to leverage new attributes in static imports without breaking old (or even not-so-old-but-not-bleeding-edge) engines? I guess so, but that implies an outer mechanism like import maps (and tooling to leverage it, especially regarding deep imports) that isn't available to an arbitrary ECMAScript implementation.

Yes, if authors want to use static imports with attributes that are supported only in new versions of hosts they would need to provide two copies of the source files. This is annoying, but it's also how every modules-related feature has worked so far. Moreover, thanks to TLA, you can try/catch at the top-level around a dynamic import with the attributes you want. This is not effectively the same as a static import (since it changes the load/evaluation order of the graph), but for most users it's a good enough approximation.

I know that "one day we will be able to polyfill this" is not a good argument for a feature (so I don't consider this as a point in favor of this design!), but the ideal future I see is to be able to virtualize and polyfill any import attribute using the compartments proposal, with something like tc39/proposal-compartments#37 (comment).

@gibson042
I'm not sure that old engines refusing to load the third example (rather than ignoring layer and executing later feature-detection-dependent fixup) is a good thing

Regarding any attribute that can be ignored and that is possible to later fix after feature detection, I'm not convinced that

import _styles from "./main.css" with { type: "css", layer: "utilities" };
const styles = layerAttributeIsIgnored ? getLayer(styles, "utilities") : _styles;

is in any way better than just not using the attribute until all the runtime you care about support it, and instead always manually applying its equivalent semantics:

import _styles from "./main.css" with { type: "css" };
const styles = getLayer(styles, "utilities");

If an attribute can be "simulated", you can just always simulate it (which is actually similar to what happens when transpiling syntax: you transpile even for modern engines, until you don't care anymore about the older ones).

@gibson042
What about building in an escape hatch? If the import itself has a means of expressing attribute-level ignorable/required status (subject to host approval, e.g. type is never ignorable), then we get behavior aligned with anticipated predominant use cases but avoid precluding future alternatives.
There's recent if somewhat distant precedent for a similar concept affecting RFC 3339 timestamp suffixes, in which a leading ! opts in to criticality such that 2022-07-08T00:14:07+01:00[knort=blargel] may be accepted while 2022-07-08T00:14:07+01:00[!knort=blargel] is rejected.

This is interesting, I wonder if it would be better to have it syntactically (with { layer?: "utilities" }, where the syntax is (Identifier | StringLiteral) `?`? `:` StringLiteral) or as part of the key itself just as a convention between hosts (with { "layer?": "utilities" }, where the syntax is (Identifier | StringLiteral) `:` StringLiteral). When I'll present this PR I'll mention that one of the design points we are reconsidering is about handling of unknown keys, and I'd be grateful if you (or someone else for you) could bring this up.

I'm curious how you think it could work "subject to host approval, e.g. type is never ignorable". If the list of ignorable (or not ignorable) keys are provided by the host, how would it work with future currently-unknown keys? If we hosts didn't introduce type now but in a few years, and they didn't know now that they would want to introduce type in the future, how could they already specify that it's not ignorable?

@justinfagnani
This point might need clarification with the update. With assertions my understanding is that type did not change the shape of the imported module - that was determined solely by the mime-type in browsers and extension in Node - it only asserted that the file type matched the import type.

This is my understanding too, and that's still how it will be implemented in HTML. I assume for Node.js will be similar, and with { type: 'json' } will not allow importing a JS file parsing it as if it was JSON.

@gibson042
Copy link

Regarding any attribute that can be ignored and that is possible to later fix after feature detection, I'm not convinced that

import _styles from "./main.css" with { type: "css", layer: "utilities" };
const styles = layerAttributeIsIgnored ? getLayer(styles, "utilities") : _styles;

is in any way better than just not using the attribute until all the runtime you care about support it, and instead always manually applying its equivalent semantics

It can definitely be better if the fixup is expensive or imperfect. Regardless, I don't want to belabor this point.

This is interesting, I wonder if it would be better to have it syntactically (with { layer?: "utilities" }, where the syntax is (Identifier | StringLiteral) `?`? `:` StringLiteral) or as part of the key itself just as a convention between hosts (with { "layer?": "utilities" }, where the syntax is (Identifier | StringLiteral) `:` StringLiteral).

Probably the latter, so it can be expressed for dynamic import in the same way—but ideally as part of the proposal itself rather than as an external convention.

When I'll present this PR I'll mention that one of the design points we are reconsidering is about handling of unknown keys, and I'd be grateful if you (or someone else for you) could bring this up.

👍

I'm curious how you think it could work "subject to host approval, e.g. type is never ignorable". If the list of ignorable (or not ignorable) keys are provided by the host, how would it work with future currently-unknown keys? If we hosts didn't introduce type now but in a few years, and they didn't know now that they would want to introduce type in the future, how could they already specify that it's not ignorable?

Having considered this further, I think the caveat might be unnecessary because any given attribute name is either known to an implementation (in which case it wouldn't be ignored) or unknown (in which case it couldn't be included in a list of non-ignorable attributes). type being in the initial proposal means that every implementation supports it so requesting ignore-if-unknown behavior is irrelevant (it's never unknown). Using your above examples, import "./image.png" with { type: "image", "as?": "arraybuffer" } would be treated like with { type: "image" } in old hosts and { type: "image", as: "arraybuffer" } in new ones, and any situation for which such variance is unacceptable would not include the ?.

An alternative design could involve consideration of not just the attribute name but also the value, such that a new implementation might want to reject import "./image.png" with { type: "image", "as?": "not-yet-shipped-format" } while an old implementation that doesn't even know about as would treat the import like with { type: "image" }. But that kind of variance doesn't seem helpful in either the name-only or the name-and-value design anyway. It's worth noting that neither design is fully general with respect to fallback, but I'm just striving for a bridge to something better while the range of valid values for any particular attribute has not been subject to many changes and thus that lack of generality isn't detrimental.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Mar 14, 2023

I'm merging this PR now to have the proposal ready for plenary next week.

@gibson042 I'm happy to continue discussing that topic in an issue!

@nicolo-ribaudo nicolo-ribaudo changed the title Proposed changes to allow assertions affect module loading Allow assertions affect module loading, and change keyword to with Mar 14, 2023
@nicolo-ribaudo nicolo-ribaudo changed the title Allow assertions affect module loading, and change keyword to with Allow assertions affect module loading change keyword to with Mar 14, 2023
@nicolo-ribaudo nicolo-ribaudo merged commit 0fbeef7 into tc39:master Mar 14, 2023
@nicolo-ribaudo nicolo-ribaudo deleted the proposed-changes branch March 14, 2023 17:11
@nicolo-ribaudo nicolo-ribaudo changed the title Allow assertions affect module loading change keyword to with Allow assertions affect module loading, change keyword to with Mar 14, 2023
@nicolo-ribaudo nicolo-ribaudo changed the title Allow assertions affect module loading, change keyword to with Allow assertions to affect module loading, change keyword to with Mar 14, 2023
@gibson042
Copy link

@nicolo-ribaudo #132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants