-
Notifications
You must be signed in to change notification settings - Fork 45
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
[Refactor] Distinguishing external "stages" from internal "phases" #122
Conversation
@@ -206,7 +206,8 @@ The following steps are taken: | |||
1. If _loader_ does not have all of the internal slots of a Loader Instance (<a href="#loader-internal-slots">3.5</a>), throw a *TypeError* exception. | |||
1. Return the result of transforming Resolve(_loader_, _name_, _referrer_) with a fulfillment handler that, when called with argument _key_, runs the following steps: | |||
1. Let _entry_ be EnsureRegistered(_loader_, _key_). | |||
1. Return LoadModule(_entry_, "ready"). | |||
1. Return the result of transforming LoadModule(_entry_, "instantiate") with a fulfillment handler that, when called, runs the following steps: | |||
1. Return EnsureEvaluated(_entry_). |
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.
loader.import()
invoke the completion of the "stages", then invoke the completion of the "phases".
1. Add new stage entry record { [[Stage]]: "translate", [[Result]]: *undefined* } as a new element of the list _pipeline_. | ||
1. Add new stage entry record { [[Stage]]: "instantiate", [[Result]]: *undefined* } as a new element of the list _pipeline_. | ||
1. Else, | ||
1. If _ns_ is not a module namespace exotic object, throw a *TypeError* exception. |
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.
bringing back the possibility of creating new ModuleStatus
objects with a module record attached to them in a synchronous way. Inspections on the entry are still async, but this helps nodejs to populate the registry.
…d when SatisfyInstance() is invoked
@@ -960,7 +956,7 @@ ModuleStatus instances are initially created with the internal slots described i | |||
1. Return _p_. | |||
</emu-alg> | |||
|
|||
<h4 id="request-instantiate" aoid="RequestInstantiate">RequestInstantiate(entry)</h4> | |||
<h4 id="request-instantiate" aoid="RequestInstantiate">RequestInstantiate(entry, buckle)</h4> |
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.
"buckle" isn't a good name. I'd just use something like instantiateSet
.
a7a1f7e
to
cfc0b68
Compare
1. Set _instance_.[[ModuleStatus]] to _entry_. | ||
1. For each _dep_ in _instance_.[[RequestedModules]], do: | ||
1. Append the record { [[RequestName]]: _dep_, [[ModuleStatus]]: *undefined* } to _deps_. | ||
1. If _instance_ is a Source Text Module Record, then: |
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.
Isn't the [[RequestedModules]] of a dynamic module always an empty list? This conditional seems unnecessary and -- from an OO perspective -- a little gross.
Just had a couple nits I left in line notes. r+ with those addressed. Thanks!! |
* removing unnecessary length property on constructors (aligning with 262). * `Module.evaluate(ns)` resolves to undefined. * no more `[[RequestedModules]]` in reflective module records. * update Realm Record term to align with 262. * mutator getters will throw. * bringing back realm as a second argument when calling `ParseModule()`.
[Refactor] Distinguishing external "stages" from internal "phases"
Features
.resolve()
or.reject()
on a ModuleStatus object. Stages are "fetch", "translate" and "instantiate".Module.evaluate(ns)
is now specced, and it is async to guarantee future proof.loader.import("name")
orModule.evaluate(ns)
.Other fixes
Invariants
SatisfyInstance()
viaawait loader.import()
,await Module.evaluate()
orget entry.module
.new Module()
), which guarantees that all namespace exotic from module records in user-land are "satisfied".new ModuleStatus(loader, key, ns)
can be used to synchronously populate the registry because all namespace objects in user-land are "satisfied".Rationale
The primary motivation is to enable synchronous registration of pre-instantiated modules. This is important for use cases like polyfills in the browser and for Node compatibility.
But we would need to be sure that such an API cannot add a not-fully-satisfied module. If we did, there would be no way for the ModuleStatus object in the registry to know where it stands in the loading pipeline. We'd have to keep back-pointers to module namespace objects' ModuleStatus objects in order to know when they are safe to be synchronously added. But there's a many-to-one relationship between ModuleStatus objects and module instances -- and it'd be nonsense to allow different ModuleStatuses to fight with each other about how to satisfy the dependencies of a module.
The way to make this coherent is to have an invariant that by the time a module instance is exposed to userland, all its dependencies have been satisfied. This keeps the roles and responsibilities of module namespace objects and ModuleStatus objects distinct, and ensures that synchronous registration is rational.
ASCII graph for this pipeline refactor