search

malaporte / nashorn-commonjs-modules

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

Support circular requires as per Node.js #14

Closed kipz closed 7 years ago

kipz commented 7 years ago

Many node modules out there contain circular require references (I know, right?), so I feel that this CommonJS loader could benefit from support for successfully resolving them like Node does.

As per: https://nodejs.org/api/modules.html#modules_cycles

Just noticed that my editor has been messing with imports. I'm happy to revert if you like.

malaporte commented 7 years ago

Thanks for the contribution! I'll have a look as soon as I have a minute.

kipz commented 7 years ago

Hey @malaporte - is there anything I can do to this PR to improve it for you?

kipz commented 7 years ago

Are you averse to using a synchronised collection instead of ThreadLocals? I think the slight overhead in normal use is dwarfed by the initial nashorn compilation times (of course it gets really fast if we call JS many times over).

I just think that if there is no significant performance impact, we should try to avoid the complexity of Thread Locals! :)

malaporte commented 7 years ago

A synchronized collection would still issues, as one running require might end up using a partially loaded module from another running in another thread, even when no cycle is involved.

kipz commented 7 years ago

@malaporte I've added use of ThreadLocals and fixed up all the other comments.

kipz commented 7 years ago

I don’t see that - are you commenting on an old commit?

On 8 Feb 2017, at 09:30, Martin Laporte notifications@github.com wrote:

@malaporte commented on this pull request.

In src/main/java/com/coveo/nashorn_modules/Module.java https://github.com/coveo/nashorn-commonjs-modules/pull/14:

@@ -36,6 +39,10 @@ public Module( this.engine = engine; this.folder = folder; this.cache = cache;

  • if(refCache == null){ This remains to be fixed I think.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/coveo/nashorn-commonjs-modules/pull/14, or mute the thread https://github.com/notifications/unsubscribe-auth/ABx1QhOQEiLbAwzQoqPwwmDTGFZSNy01ks5raYs1gaJpZM4Lq79W.

malaporte commented 7 years ago

Ah indeed I didn't read the code correctly - nevermind me then.