-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Normative: Add dynamic import() #1482
Conversation
Tests: tc39/test262#1164 |
<p>The intent of this specification is to not violate run to completion semantics. The spec-level formalization of this is a work-in-progress.</p> | ||
</li> | ||
<li> | ||
Every call to HostImportModuleDynamically with the same _referencingScriptOrModule_ and _specifier_ arguments must conform to the <em>same</em> set of requirements above as previous calls do. That is, if the host environment takes the success path once for a given _referencingScriptOrModule_, _specifier_ pair, it must always do so, and the same for the failure path. |
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.
Does this also hold with a null referencingScriptOrModule? ie, is everything with null
considered the same referencing module, or is it more like NaN where they’re all intended to be distinct?
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.
Yes, it does hold. Do you think this is unclear in the text?
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.
My reading is that for a given referencing location, two calls should be idempotent, but that one call from each of two different locations need not produce the same result.
This line suggests that one call from each of two different locations with null will always be required to have the same result - ie, it seems stricter on engines for when there’s no referencing location. The difference between the cases strikes me as odd; it seems like either the null case needs relaxing or the non-null case needs tightening, to make things more consistent?
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.
Well, to explain how it works out in HTML (cc @domenic), if you are in one of these null contexts, then it's basically like you're in the context of the whole web page. So the base URL of the whole page is used to parse relative URLs. In this case, it does make sense to group together all the null imports.
I suspect it might work out similarly in other environments: When you don't have a clear referencing script, the context is something corresponding to the Realm. But it may be that there are other environments that need a weaker condition.
How about we start with this strong condition, and consider a follow-on needs-consensus PR to weaken the condition if an embedding environment comes along where this invariant needs to be relaxed? Stating as strong conditions as possible could keep things more predictable for ECMAScript itself.
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.
Sounds good - if the intended usage in browsers is indeed to group all nulls as the same effective referencing script, then as-written seems fine to me.
spec.html
Outdated
@@ -23295,6 +23394,10 @@ <h1>Forbidden Extensions</h1> | |||
<li> | |||
When parsing for the |Module| goal symbol, the lexical grammar extensions defined in <emu-xref href="#sec-html-like-comments"></emu-xref> must not be supported. | |||
</li> | |||
<li> | |||
<ins>|ImportCall| must not be extended.</ins> | |||
<!-- This is so that in the future we can potentially add new arguments or support ArgumentList. --> |
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.
Given this intended support, should a trailing comma be allowed in the grammar? (not an editor question, just a curious delegate question)
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.
Hmm, I guess I have to say that, personally, I would expect a trailing comma to be allowed, not because of potential future extensions, but just because we allow trailing commas in ordinary calls. However, I think we should pursue this possible change in a separate needs-consensus PR, as the grammar in this proposal is already widely implemented and shipped, and it was promoted to Stage 3 with the interaction visible to the committee.
spec.html
Outdated
If a Module Record corresponding to the pair _referencingScriptOrModule_, _specifier_ does not exist or cannot be created, an exception must be thrown. | ||
</li> | ||
<li> | ||
This operation must be idempotent if it completes normally. Each time it is called with a specific _referencingScriptOrModule_, _specifier_ pair as arguments it must return the same Module Record instance. |
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.
confirming; is "idempotent" the correct word here? cc @michaelficarra / #1133
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.
I change the equivalent to this line to avoid the usage of "idempotent" this way in #1363. We'll need to resolve the conflict.
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.
Any suggestions for the wording here?
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.
This operation must be deterministic with respect to its parameters if it completes normally
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.
Some embedding environments may do some I/O here, e.g., to get the module, so I don't think this would be accurate.
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.
I just don't understand what the term "deterministic" would mean here. When I read that sentence, it makes me think that it's saying that the operation is a function of its parameters, which would not be the case.
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.
@littledan It means that if I give you the same parameters in two separate calls to that function, it should produce the same result. If you'd prefer an alternative way to state that, you should comment on #1363.
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.
I agree with @littledan. "deterministic" does not have that meaning. A deterministic function could have inputs other than its formal parameters, such as the phase of the moon, the load on the CPUI, or the load factor on a cache. It may well be "deterministic" what will happen in all those cases but that doesn't guarantee that successive calls with the specific argument will return the same result.
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.
@allenwb That's why the phrase is "deterministic with respect to", to tell you what are the inputs for that determinism.
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.
Let's move this discussion to #1363. I'll update this patch when we land it.
Thanks @jmdyck , made the suggested fixes. |
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.
I looked it over again, but don't have anything to add.
This PR includes minor changes to rebase, which are out for review in tc39/proposal-dynamic-import#72 Co-authored-by: Domenic Denicola <[email protected]>
This PR includes minor changes to rebase, which are out for review in tc39/proposal-dynamic-import#72 Co-authored-by: Domenic Denicola <[email protected]>
This PR includes minor changes to rebase, which are out for
review in tc39/proposal-dynamic-import#72
Proposal author: Domenic Denicola