malaporte / nashorn-commonjs-modules

CommonJS modules support for Nashorn
MIT License
108 stars 31 forks source link

Question about the content of module and module.children #27

Open arisone opened 6 years ago

arisone commented 6 years ago

I have the following script:

var module1= require('./module1);
var instance1= new module1.myclass1();
...
var module2= require('./module2);
var instance2= new module2.myclass2();

I pass the script to the engine (eval) and then I map each instance to a java class with the method getInterface(object, class).

The two modules and the two instances are created in the engineScope. In the Module.class there is this part:

  private Module compileJavaScriptModule(Folder parent, String fullPath, String code)
      throws ScriptException {
    Bindings engineScope = engine.getBindings(ScriptContext.ENGINE_SCOPE);
    Bindings module = createSafeBindings();
    module.putAll(engineScope);

and then this other part:

  public Object require(String module) throws ScriptException {
    ...
    try {
      // If not cached, we try to resolve the module from the current folder, ignoring node_modules
      if (isPrefixedModuleName(module)) {
        found = attemptToLoadFromThisFolder(resolvedFolder, filename);  // this calls the method compileJavaScriptModule
      }
    ...
    children.add(found.module);

So at the end I have module1 and instance1 among the children of module2. Obviously I have also all other things present in the engineScope (e.g. require, exports, and module <main>). The more I load module the more the structure becomes redundant.

Considering that I am new to Nashorn and node.js and there is no many information about module.children I'm questioning if it is correct to put the full content of the engineScope as child of a module.

Thanks for your attention. aris

P.S. I've tried to comment module.putAll(engineScope): it works even so and the content of children seems more correct to me because I see only the module objects required by the one I'm inspecting.

arisone commented 6 years ago

I've done a little test to explain well what I mean. I've created 4 modules (written in typescript and transpiled with module set to commonjs). The relation between modules is as follow:

The test:

The test has been run both with the original code of nashorn-commonjs-modules and with the module.putAll(engineScope) commented.

The attached zip contains the java main to run the test, the four js modules, the two json produced from the runs.

modules-test.zip

arisone commented 6 years ago

I've found the moment when the change has been made: Use real JS object whenever we need an instance of Bindings. but I don't understand why the module.putAll(engineScope) has been added. Thanks for your attention.

malaporte commented 6 years ago

Sorry, I haven't been much responsive, as mentioned in another issue I'm deep into house renovation work 😁

From what I remember, I added this part to copy the content of the global scope, because NashornScriptEngine.createBindings does some special magic when not running with the --global-per-engine flag (the default). Aka it allocates a brand new Global. But I've just tested with the line commented out in both modes, and all the UTs are passing, so I'm a bit confused. Certainly I haven't done this for no reasons...

I'll put those changes in a PR and do some more testing.

arisone commented 6 years ago

Don't worry @malaporte and thanks for your response. I'd like to contribute.

I've read this (Nashornjsr223 enginenotes --global-per-engineoption) and some other posts on the mailing list of Nashorn.

I've examined the diff between the current and the previous version of Module. In the previous version, the call engine.createBindings was used to retrieve main objects (JSON, Object, Error) but in the current version they have been substituted by Module.createSafeBindings which internally calls objectConstructor.newObject().

The reference to objectConstructor and the others to main objects (engine included) are propagated from parent module to its children so the objects used are always the same. Furthermore engine.createBindings is no longer invoked, avoiding any special treatments which depend on engine settings. This should explain why the UTs are passing. For my current understanding it's similar to have the global-per-engine always set (in this way also the name of the method createSafeBindings make more sense to me).