Opened 2 years ago

Last modified 2 years ago

#6472 assigned enhancement

ES Module Support

Reported by: smiley Owned by: smiley
Priority: Nice to Have Milestone: Backlog
Component: Core engine Keywords: SpiderMonkey
Cc: Patch:

Description

It would allow for a less cluttered and explicit usage of externally defined code. Moreover, it would simplify the current global scripts system which just execute the scripts in the specified directory in the same scope. This leads to cases where pseudo-libraries such as the rmgen have to bootstrap itself with Engine.LoadLibrary by recursively calling it for all subdirectories to make sure that all definitions are available. And there aren't any way to prevent double execution, so its a requirement to ensure that Engine.LoadLibrary will never be run with the same directory during execution of the map script unless the scripts don't mutate any state. Even so, double execution is a waste. This is a real possibility if there are dependencies between two separate directories. Engine.LoadLibrary("rmgen") and Engine.LoadLibrary("someother_lib_for_math_or_something") perhaps if they needed to pull in rmgen/math.js.

To summarize, it would enable import and export which would allow for cleaner code that behaves in a more predictable and familiar manner to modern JavaScript that runs in NodeJS or the similar. Which brings in yet another advantage as it would allow to execute such code in NodeJS or the browser. A browser embeddable rmgen if you will.

The changes required for this while not extremely simple are well documented.

Change History (4)

comment:1 by Stan, 2 years ago

That sounds like a good idea. Have more explicit everything would be nice. I don't know how feasible it is with our current programmer roster,though.

comment:2 by smiley, 2 years ago

Owner: set to smiley
Status: newassigned

The hooks needed for module resolution has been done and seems to work. I have tested a chain of imports and both the code in the module is executed and the correct values are resolved. This was done by loading a map script as an ES module which also showed that cross file global variables are not exposed.

The code for that is encapsulated in a ScriptModuleLoader class which is owned by the ScriptInterface. This class keeps track of the current module root which would be the path for the top module and a map of resolved modules, which is nothing fancy, just std::map<VfsPath, JS::PersistentRootedObject>.

Buggy behavior discovered so far is that while exceptions are properly thrown and displayed, the module path is garbage. There is the right error message displayed but the stack is not logged correctly. This needs more investigation as I quickly looked at ScriptException::CatchPending and nothing too obvious was found.

Last edited 2 years ago by smiley (previous) (diff)

comment:3 by smiley, 2 years ago

Something else open for debate is whether or not the filesystem should support relative path notation. So far, such requirements have not existed.

I am not sure that it should to be honest. Paths should probably be resolved in the module loader. For convention, while unnecessary singular . should also be supported I guess along with the required ...

comment:4 by Stan, 2 years ago

Relative path support sounds dangerous, error prone, especially if it's only supported for this. I think standard vfs paths make sense, maybe omitting the root folder e.g. gui or maps/random/.

With regards to performance, does this have any impact ? Nobody ever tried minification or google closure to see if that changed performance. The later is a bit tricky since it requires some hacks.

Note: See TracTickets for help on using tickets.