malaporte / nashorn-commonjs-modules

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

Search order doesn't seem to match NPM's #5

Closed szeiger closed 8 years ago

szeiger commented 8 years ago

This is a bit speculative because I haven't actually verified it with nashorn-commonjs-modules. I have since ported the project to Scala, adapted it better to my use case, and fixed the bugs I ran into. It is not a single file with ~150 LOC. I discovered this issue with my version but looking at the source code tells me that I've reproduced the logic faithfully and therefore the original implementation should also fail.

I am trying to load the mermaid NPM module. It contains a file src/d3.js:

//log.debug('Setting up d3');
var d3;

if (require) {
  try {
    d3 = require('d3');
  } catch (e) {
    //log.debug('Exception ... but ok');
    //log.debug(e);
  }
}
...

Due to the order in which resources are searched, the call require('d3') gets resolved back to the current file. I haven't done any research of how this supposed to work on NPM but it seems that an "obvious" node_module reference should first be searched in the global modules, not moving upwards from the current directory which may contain a .js file with the same name. After changing the search order to check the root folder first and only work back up from the current folder if it's not found at the root, I can load these modules correctly.

szeiger commented 8 years ago

I went straight to the source and implemented the algorithm from https://nodejs.org/api/modules.html#modules_all_together. This works correctly in this case. I had to do one small modification from the documented algorithm to make progress loading domino. It uses a package.json with main set to ./lib and expects this to load ./lib/index.js. Since this is packaged as a node module and available on NPM I assume that this works on node, so I modified the algorithm to also try loadAsDirectory on main when loadAsFile fails.

My current Scala implementation is here: https://gist.github.com/szeiger/bf6d5c24b4b1e0c020b039205935c9ec

malaporte commented 8 years ago

My implementation already won't try to load an non-prefixed module name from the local folder, so the issue you describe doesn't occur. Still I'm going to rework the algorithm so that it's easier to confirm that it respects the same algorithm as the one implemented by Node (right now there are indeed a few discrepancies).

malaporte commented 8 years ago

There, I changed the code a bit, as far as I know it mirrors exactly the algorithm described in NodeJS doc.