-
Notifications
You must be signed in to change notification settings - Fork 14
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: Define module root level context #98
base: master
Are you sure you want to change the base?
Conversation
spec.html
Outdated
</dl> | ||
<p>An implementation of GetHostDefinedAsyncContextMapping must conform to the following requirements:</p> | ||
<ul> | ||
<li>It must perform and return a List of Async Context Mapping Records.</li> |
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.
Another requirement would be to always return the same mapping.
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.
or in the opposite direction, pass the module record to it.
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.
Updated with a parameter module records and the result list must contain the same mapping for the given module record.
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'm not sure I understand this change. I thought the goal of this PR was to make is so that all modules have the same top-level mapping, while this PR is now defining the top-level mapping as potentially different for each module.
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 allows hosts to add mapping entries based on the origin of the module records. The goal of the PR is allowing hosts initialize their entries, instead of deriving the mappings based on the originator while an imported module is only evaluated once.
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.
So this allows hosts to implement both fresh-context-per-module and module-based-on-importer?
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.
Not sure what you mean by module-based-on-importer, but my understanding is that if two different importers import the same module, you'd still only evaluate once, and any importer dependency would resolve based on one or the other (presumably the earlier one).
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.
Right, the original motivation of this PR was to give an answer to this question: what is the context of the module in this case?
variable.get(); // undefined
variable.run(1, () => import("x"));
At the top-level of x
, does variable.get()
return undefined
or 1
? This PR as-is does not give an answer, and instead laves it host-defined.
To give an answer, this PR could either:
- not do anything, and
import("x")
would see1
- have a non-per-module top-level snapshot (stored in the realm record?), and
import("x")
would seeundefined
With this PR both behaviors are possible:
- if the host implements
HostGetTopLevelAsyncContextMapping(module)
as "Return AsyncContextSnapshot()", then it's1
- if the host implements
HostGetTopLevelAsyncContextMapping
as "Return the original top-level snapshot", then it'sundefined
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.
A couple nits, but overall it looks good.
<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p> | ||
<ul> | ||
<li>It must return a List of Async Context Mapping Records.</li> | ||
<li>The result List must contain the same Async Context Mapping Records for a given module record.</li> |
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.
FWIW, HostHasSourceTextAvailable has a similar requirement worded as "It must be deterministic with respect to its parameters. Each time it is called with a specific func as its argument, it must return the same result". It might be worth rephrasing to be consistent with that language.
</dl> | ||
<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p> | ||
<ul> | ||
<li>It must return a List of Async Context Mapping Records.</li> |
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 still think this requirement should be deleted, since it's completely redundant with the signature. Looking through the other host-defined ops, I see a few with non-unused returns (e.g. HostHasSourceTextAvailable and HostResizeArrayBuffer) and none of them have this sort of restatement of the signature as an enumerated requirement.
<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p> | ||
<ul> | ||
<li>It must return a List of Async Context Mapping Records.</li> | ||
<li>The result List must contain the same Async Context Mapping Records for a given module record.</li> |
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 host hook is only called once per module record, so this requirement is not necessary.
This defines the root level context for a SourceTextModule. This makes the surrounding context consistent and predictable when module evaluation is either deferred, or dynamic. When a module evaluation is deferred, the module's body only gets evaluated when its export entries get accessed. However, this depends on if the module contains TLA. Hosts, that want to expose the originating context of a module, can expose the context via HostGetImportMetaProperties, and HostFinalizeImportMeta.
Notably, this doesn't change the context of ScriptEvaluation, as it is used in hosts like https://html.spec.whatwg.org/#timers:run-a-classic-script.
Fixes #93