Module Records need to be an abstract "base class" data structure, with only the dynamic properties of a module:
[[Realm]] Realm Record
[[Environment]] Lexical Environment or undefined
[[Namespace]] Object or undefined
[[Evaluated]] Boolean
(The [[Realm]] property is described in bug 3746.) The spec defines just one "subclass" of Normal Module Records, which extends Module Records with the properties computed from source code:
[[ECMAScriptCode]] Parse result
[[ImportedModules]] List of String
[[ImportEntries]] List of ImportEntry Records
[[LocalExportEntries]] List of ExportEntry Records
[[StarExportEntries]] List of ExportEntry Records
[[IndirectExportEntries]] List of ExportEntry Records
The following top level abstract spec operations become overrideable methods on the Module Record class:
* GetExportNames ==> Module Record :: GetExportNames
* ResolveExport ==> Module Record :: ResolveExport
* ModuleDeclarationInstantiation ==> Module Record :: Instantiate
* ModuleEvaluation ==> Module Record :: Evaluate
See https://gist.github.com/dherman/cad85565e0eb16d0a22d for details.
Dave
I think I going to call "Normal Module Records" "Source Code Module Records" as that is a bit more descriptive concerning what they correspond to.
Putting on this bug since it concerns Normal Module Records methods but it's really broader then that.
Why do you have both NormalModule.instantiate() and NormalModule.evaluate() calling HostResolveImportedModule to get the imported module records.
I presume that you assume (actually require) that any given request pair will produce the same module record from both call sites. Why isn't it better to only resolve them once in instantiate()?
Also (if I'm interpreting things correctly) a consequence of not resolving the [[ImportedModules]] list in or prior to instantiate() is that any module imported solely via something like:
import "someModuleNzame";
may not get resolved for the first time until evaluate() tries to resolve it because such imports do not generate ImportEntry records. That doesn't feels right. Shouldn't the resolvability of all imports have been determined before evaluation starts?
In my version this resolution took place in ParseModuleAndImports and all the Impurt and Export Entry records got updated to include module record references in additon to their initial string name references.
Another consequences is that I believe it means that the initial resolution of a module may not occur in the same order as references to them occur in the module source code. I'm not sure this makes a difference (because initialization still uses the module list order) but being unsure make me uncomfortable ;-)
Some issues about GetExportedNames
Do you really want to throw a SyntaxError on Cingular star exports? they seem benign. For example consider:
"mod.js":
export const x=42;
export * from "mod.js"; //export * from ourselves
The export * doesn't add anything, but it really doesn;t hurt anything as long as the specification/implementation protect themselves from looping
A agree that we want to get rid of ambiguous export * names. But I believe that you algorithm is not so nice with this case:
"mod2.js":
export * from "mode3.js";
export * from "mode3.js"; or "../mode3.js", etc
You algorithm deletes all names exported from a module if more than one export * resolves to it. It seems to be that the handling of this situation should be either:
1) ignore the redundant export *
2) throw a syntax error (in the spirit of the previous issue above).
But, it's not that easy to detect either redundant export *, nor is it clear that they should be an error. Consider:
"mod3.js":
export * from "mod4.js";
export * from "mod5.js";
"mod4.js":
export * from "mod5";
mod5.js":
export const foo=42;
It doesn't seem reasonable that the author of mod3 should have to know that mod4 is also exporting the mod5 names.
GetExportedNames is only used (at least in the ES6 spec.) to setup module namespace objects. And I agree that we don't want ambiguous names to pass 'in' tests like you showed. It seems that a solution to all of this is for GetExportedNames to only filter duplicate export * names but not to check for ambiguous star * exports. Then GetModuleNamespace could use ResolvedExport to check for any ambiguous star * names and remove those from the names bound by that module namespace.
> GetExportedNames is only used (at least in the ES6 spec.) to setup module
> namespace objects. And I agree that we don't want ambiguous names to pass
> 'in' tests like you showed. It seems that a solution to all of this is for
> GetExportedNames to only filter duplicate export * names but not to check
> for ambiguous star * exports. Then GetModuleNamespace could use
> ResolvedExport to check for any ambiguous star * names and remove those from
> the names bound by that module namespace.
I agree that these overlapping re-exports that agree on provenance are benign, and this plan makes sense to me. Thanks!
If I'm reading right, the latest draft spec is still throwing for these cases and will need to be relaxed to just return an empty list from GetExportedNames when it hits a cycle. I'll re-review the next draft for bugs, but from what I can tell we're pretty much down to minor bugs and no serious design issues at this point.
Dave
fixed in rev34 editor's draft
fixed in rev34