-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6c6c769
to
f5e0530
Compare
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*, « a newly created *TypeError* object »). | ||
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_. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
When you say "Throw on unsupported assertions", obviously this is what |
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 |
The same way as any new syntax, right? Compilers will have to downlevel new keys and new values for older browsers. |
This is syntax, and any new syntax can't generally be added in a nonbreaking way. Things that aren't supported shouldn't parse. |
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. |
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 |
I’m asking about keys, not values. From https://github.com/tc39/proposal-import-assertions/#proposed-syntax:
And in this PR, in https://github.com/tc39/proposal-import-assertions/pull/131/files#diff-7d681727fcf47dc0b9a7512a470fb0da63276c625891a5cc232d725bd12912fdR139:
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 I would also appreciate an answer about whether |
@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. |
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.
We have thought about this for a while, and there are three options for unknown keys:
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:
All these example attributes cannot be simply ignored:
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.
Yes, For other keys, it depends. For example, a possible 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
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:
|
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.
Thank you for the examples. I think the middle one might be better spelled with distinct
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. There's recent if somewhat distant precedent for a similar concept affecting RFC 3339 timestamp suffixes, in which a leading |
This point might need clarification with the update. With assertions my understanding is that That changes a bit if HTML uses ie, for CSS you might have, |
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 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).
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).
This is interesting, I wonder if it would be better to have it syntactically ( I'm curious how you think it could work "subject to host approval, e.g.
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 |
It can definitely be better if the fixup is expensive or imperfect. Regardless, I don't want to belabor this point.
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.
👍
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). 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 |
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! |
with
with
with
with
with
with
with
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
avoid assert { type: "json" } / with { type: "json" } change tc39/proposal-import-attributes#131 Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com> Signed-off-by: Andrew Stein <steinlink@gmail.com> # Conflicts: # .github/workflows/build.yml
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"
vstype: "css-module"
) and it's better to error early.Change the keyword from
assert
towith
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
towith
.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 withassert
as an alias towith
, and if this migration happens successfully we'll be able to remove theassert
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