Normative: Prohibit module attributes as cache keys #66
Conversation
This patch changes the invariants on hosts, requiring that module attributes do not affect the interpretation of the module. Instead, they may only be used for checks. This constraint may be relaxed in the future, or willfully violated by hosts, but there would be risk in doing so; an extensive note is added explaining this risk. Addresses #64
spec.html
Outdated
|
|
||
| <emu-note type=editor> | ||
| <p>The above text implies that, if a module is imported multiple times with different _moduleRequest_.[[Attributes]] values, then there can be just one possible "successful" value (possibly as a result of multiple different attributes), but that it can also fail with an exception thrown; this exception from one import does not rule out success with a different attribute list.</p> | ||
| <p>The restriction of of attributes to not affect the contents of the module or be part of the cache key is sometimes referred to as permitting only "check attributes" and not "evaluator attributes", where the latter would change the contents of the module. Future versions of this specification may relax this restriction, and it's understood that some hosts may be tempted to willfully violate this restriction, but the module attributes champion group advises caution with such a move. There are three possible ways to handle multiple imports of the same module with different attributes, if the attributes cause a change in the interpretation of the module:</p> |
|
To be clear, this PR does not limit attributes from being used in resolution. I cannot see how we would ever limit such a thing in text if we provide the attribute prior to resolution. I think this is fine, but would still caution against doing things like modifying the |
|
if they won't change resolution, we can require that within a module, every occurance of a specifier has to have matching attributes. |
|
I could imagine that in the future you may want to have alternative resolution based upon module attributes such as some kind of |
This patch allows hosts to do this. However, it also lets hosts do things like make |
Well, it prohibits the attributes from being used in the calculation of the cache key. But, on another read, this PR fails to prohibit hosts from the "race" case, where the attributes from the first time it's imported affect its interpretation, in a sticky way, which is ignored by future imports (or, those future imports get errors). I do want to prevent that case; sounds like we should iterate on the wording. |
Also delete of of
|
I've changed the wording a bit to make the "check" restriction more explicit. I think this would imply that attributes can't be used in resolution. (Also deleted the "of of".) Reviews welcome! |
|
@littledan to clarify, i mean disallowing this within the same module/file: import 'x.mjs' with type: 'json';
import 'x.mjs';multiple modules can still compose together with different attributes |
|
@devsnek I guess hosts would be allowed to make that restriction by keeping track of which attributes it got for which specifier+referrer pair. I don't understand the motivation for the restriction, but it's not prohibited by the spec. |
Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
|
Thanks @ljharb ; these seem like reasonable clarifications. |
| Each time this operation is called with a specific _referencingScriptOrModule_, <del>_specifier_,</del> <ins>_moduleRequest_.[[Specifier]]</ins> pair as arguments it must return the same Module Record instance if it completes normally. | ||
| </li> | ||
| <li> | ||
| <ins>_moduleRequest_.[[Attributes]] must not influence the interpretation of the module or the module specifier; instead, it may be used to determine whether the algorithm completes normally or with an abrupt completion.</ins> |
There was a problem hiding this comment.
What I'm trying to capture here is for a given attribute, it should either provide the same Module, or an error, whether the attribute is absent or present, and with any value in the attribute. Thoughts on this or alternative text?
| <ins>_moduleRequest_.[[Attributes]] must not influence the interpretation of the module or the module specifier; instead, it may be used to determine whether the algorithm completes normally or with an abrupt completion.</ins> | |
| <ins>_moduleRequest_.[[Attributes]] must not influence the interpretation of the module or the module specifier; instead, it may be used to determine whether the algorithm completes normally or with an abrupt completion. This applies to any individual attribute pair, as well as the overarching _moduleRequest_.[[Attributes]] group.</ins> |
There was a problem hiding this comment.
I'm not sure I understand this addition. " moduleRequest.[[Specifier]] pair" mentions a single thing (the specifier) and then appends "pair". Was this meant as the list of pairs made up of the specifier and each attribute..? I sounds like "no combination of presence or absence of attributes" which seems like a more complicated framing of the original sentence (which already rules out [[Attributes]] being considered at all). Is there an example/loophole you had in mind here?
There was a problem hiding this comment.
The term is used 3 or 4 other times in this PR alone; it means to me "a single attribute name and value".
The intention is that each attribute is considered independently for this invariant - it may be true that it's not necessary to do so and the invariant is still preserved, but it feels closer to the underlying intention to me to be specific.
There was a problem hiding this comment.
I think the only place with pair I'm seeing is:
referencingScriptOrModule, moduleRequest.[[Specifier]] pair
Which is the pair of importer and specifier. Is that what you meant here? If it's about attribute name/value pairs, I assume it should reference _moduleRequest_.[[Attributes]], not _moduleRequest_.[[Specifier]].
There was a problem hiding this comment.
ohhh, thanks for clarifying. will fix the suggestion.
|
This disable the possibility that the http server return different file for the same path based on the attributes |
|
@Jack-Works yes, that is part of the intention. |
Yes, that's true. We could consider weakening this requirement during Stage 2, if we find that the header is required. The current plan of record does not include this header, but I'm open to it, based on the feedback we get from WHATWG. |
|
I presented this PR to TC39, and it was part of the Stage 2 consensus, so landing it. |
This patch changes the invariants on hosts, requiring that module
attributes do not affect the interpretation of the module. Instead,
they may only be used for checks. This constraint may be relaxed
in the future, or willfully violated by hosts, but there would be
risk in doing so; an extensive note is added explaining this risk.
Addresses #64