> I just put out the second implementation patch at<https://bugzilla.mozilla.org/show_bug.cgi?id=540724>, against current CVS HEAD. Check it out if you're interested. I have worked on this for I most of my free time I can have for coding in the last 12 days, and have arrived at a much better design than what was in the first attempt. I attached a comment to the Bugzilla issue describing the design decisions I took. It all feels round to me at the moment. I took care to document all classes and interfaces in great detail. I'll proceed with writing tests for it, but if you don't mind reviewing and trying bleeding-edge stuff, go for it.
Hi Attila and list,
I've started playing with the patch in the context of my ESXX project and while I haven't had time to finish yet I have a couple of questions:
1) I already have a path array in ESXX, which also happens to be a JS array that is searched from the front. However, having two path objects is weired, so I'm wondering if we could either perhaps replace the 'secure' argument with a 'paths' argument (Scriptable) so I can inject mine, or add a method that returns Require's paths object so I can use that one instead?
2) I have a special property named 'esxx.location' that contains the URI of the currently executing file. For this to work I need to know when the Script is executed. It could be solved by making Require.executeModuleScript protected, or better, adding an exec method to ModuleScript.
> 1) I already have a path array in ESXX, which also happens to be a JS > array that is searched from the front. However, having two path objects > is weired, so I'm wondering if we could either perhaps replace the > 'secure' argument with a 'paths' argument (Scriptable) so I can inject > mine, or add a method that returns Require's paths object so I can use > that one instead?
(I just realized that my array is not an array of string but URI objects, so I think I need to provide my own Scriptable that implements the correct view of the path array.)
Yeah, returning null in ScriptProviders and ScriptSourceProviders allows them to be composed and fall back on each other without exceptions being thrown and caught. It's only when it'd surface as nonexistent from require() that the error is thrown.
> Ok, you throw exception in Require :), in getModule() :) nice trick
> Jarosław Pałka pisze: >> Attila,
>> I applied third version of your patch. I have two comments: >> - I don't see where you execute interoperablejs tests, I see them in source code but it looks like they are not run as part of junit-all target Maybe I missed something :( >> - In CachingModuleScriptProviderBase, in line 70, you return null when module source is null, according to CommonJS spec we should throw error when module was not found
>>> Also, just this morning i put out a new patch at <https://bugzilla.mozilla.org/show_bug.cgi?id=540724> that already has all the tests I wrote in it - this likely makes your work unnecessary; I'm sorry I didn't wait on you, but I have scarce spare time to work on this, so when I do have, I take advantage of it... By all means though, give it a review. I did end up taking your approach and including the files, as the new set of files wasn't available through command line SVN...
>>> Anyway, others might want to take the opportunity to check the current patch - it's now tested with 60% coverage; the untested stuff is mostly to do with cache revalidation. It passes all the compliance tests + my additional implementation tests.
>>> Also, a big improvement compared to the previous patch is that I made require() threadsafe, with good concurrent use properties, while not sacrificing performance for single threaded use. So it can now be used to load modules on demand into a shared top-level scope (which is something many server-side JS embeddings do).
> I applied third version of your patch. I have two comments: > - I don't see where you execute interoperablejs tests, I see them in source code but it looks like they are not run as part of junit-all target Maybe I missed something :(
No, I think it's me who missed something - in testsrc/build.xml, just under:
> - In CachingModuleScriptProviderBase, in line 70, you return null when module source is null, according to CommonJS spec we should throw error when module was not found
>> Also, just this morning i put out a new patch at <https://bugzilla.mozilla.org/show_bug.cgi?id=540724> that already has all the tests I wrote in it - this likely makes your work unnecessary; I'm sorry I didn't wait on you, but I have scarce spare time to work on this, so when I do have, I take advantage of it... By all means though, give it a review. I did end up taking your approach and including the files, as the new set of files wasn't available through command line SVN...
>> Anyway, others might want to take the opportunity to check the current patch - it's now tested with 60% coverage; the untested stuff is mostly to do with cache revalidation. It passes all the compliance tests + my additional implementation tests.
>> Also, a big improvement compared to the previous patch is that I made require() threadsafe, with good concurrent use properties, while not sacrificing performance for single threaded use. So it can now be used to load modules on demand into a shared top-level scope (which is something many server-side JS embeddings do).
> On 31/01/2010 20:15, Attila Szegedi wrote: >> Hi all,
>> I just put out the second implementation patch at<https://bugzilla.mozilla.org/show_bug.cgi?id=540724>, against current CVS HEAD. Check it out if you're interested. I have worked on this for I most of my free time I can have for coding in the last 12 days, and have arrived at a much better design than what was in the first attempt. I attached a comment to the Bugzilla issue describing the design decisions I took. It all feels round to me at the moment. I took care to document all classes and interfaces in great detail. I'll proceed with writing tests for it, but if you don't mind reviewing and trying bleeding-edge stuff, go for it.
> Hi Attila and list,
> I've started playing with the patch in the context of my ESXX project and while I haven't had time to finish yet I have a couple of questions:
> 1) I already have a path array in ESXX, which also happens to be a JS array that is searched from the front. However, having two path objects is weired, so I'm wondering if we could either perhaps replace the 'secure' argument with a 'paths' argument (Scriptable) so I can inject mine, or add a method that returns Require's paths object so I can use that one instead?
You can easily obtain the paths of a require instance. Remember, a require() function is just a JS object, so you can do:
> 2) I have a special property named 'esxx.location' that contains the URI of the currently executing file. For this to work I need to know when the Script is executed. It could be solved by making Require.executeModuleScript protected, or better, adding an exec method to ModuleScript.
I see - you need a post-exec hook. Okay, that's a reasonable thing. I actually at one point in time had ModuleScript.exec(), but I scrapped it. The reason is in the way SoftCachingModuleScriptProvider is implemented; if you look into it, ModuleScript instances in it are not even preserved, but they are deconstructed/reconstructed into/from the ScriptReference as needed. This is done so that there's no strong reference Script objects being held, so unused scripts can indeed get unloaded. (functions have pointers to their parent scripts, so as long as there's any function from the script reachable in any of the currently running JS program instance, they won't get GCed, otherwise they can be). For the same reason, you shouldn't create such post-exec hook by creating a wrapper around Script that itself implements Script with pre/post processing - you'd screw up soft caching as you can't sneak in a strong reference to your wrapper Script object into the Script object that Rhino compiles for you, which'd be required to ensure soft cache doesn't load multiple reachable copies of scripts (& functions).
So, an explicit way to register a post-exec hook is the cleanest solution - I'll look into where would this fit best.
Thank you very much for taking time to look into this and make suggestions.
Given that the current implementation state seems sufficiently stable, I committed it to CVS HEAD and will continue development there.
The committed version features following changes compared to last patch:
- Require constructor can now accept a pre-exec and a post-exec Script; these are executed before and after any module script, respectively. This is in response to Martin Blom's requirement for a post-exec hook. Martin, can you confirm this is something that suits your needs?
- RequireBuilder has been moved to the base package, seeing how Require constructor now takes quite a lot of arguments (which, for all apparent unwieldiness, is how I like it, as I love immutable objects when possible). I expect people will prefer to use RequireBuilder instead of invoking the Require constructor directly, although of course, you can still do that just as well.
- Renamed the org.mozilla.javascript.commonjs.module.support package to *.provider, as all the classes in it actually deal with module provider implementation (now that RequireBuilder has been moved out of it).
- added a MultiModuleScriptProvider, for easy composition of multiple module source providers into a single one. This is meant to open the door for modular implementation of advanced module provision methods, above and beyond loading them from .js source files. I.e. Jarosław's idea of annotations-based Java-defined modules can be implemented as a separate provider, and composed with a more conventional provider into a single composite using the MultiModuleScriptProvider. Jarosław, care to provide the appropriate ModuleScriptProvider implementation?
Next on the roadmap is using the stuff available so far to add require() to Rhino shell. For compatibility, it'll probably require a new command-line switch and be off by default. I'll outline the further ideas soon.
> Also, just this morning i put out a new patch at <https://bugzilla.mozilla.org/show_bug.cgi?id=540724> that already has all the tests I wrote in it - this likely makes your work unnecessary; I'm sorry I didn't wait on you, but I have scarce spare time to work on this, so when I do have, I take advantage of it... By all means though, give it a review. I did end up taking your approach and including the files, as the new set of files wasn't available through command line SVN...
> Anyway, others might want to take the opportunity to check the current patch - it's now tested with 60% coverage; the untested stuff is mostly to do with cache revalidation. It passes all the compliance tests + my additional implementation tests.
> Also, a big improvement compared to the previous patch is that I made require() threadsafe, with good concurrent use properties, while not sacrificing performance for single threaded use. So it can now be used to load modules on demand into a shared top-level scope (which is something many server-side JS embeddings do).
>> W dniu 8 lutego 2010 10:52 użytkownik Attila Szegedi >> <szege...@gmail.com> napisał: >>> I don't have a problem with depending on external SVN repo. It's unlikely that a googlecode SVN will go anywhere. The Rhino build already pulls libraries from Maven repositories. I actually started by adding:
>>> This makes it reside in build/test/interoperablejs-read-only. Alternatively, we could have it go under testsrc/org/mozilla/javascript/commonjs/module. But I'm really not a fan of hosting 3rd party code in Mozilla CVS, for legal reasons. Yes, I know InteroperableJS is MIT licensed, but still.
>>> Attila.
>>> On 2010.02.08., at 9:48, Jarosław Pałka wrote:
>>>> Before I prepare my patch we need to decide where to put >>>> interoperablejs. I don't think that having dependencies to external >>>> SVN repo is a good solution. I think the best way is to simply commit >>>> content of http://interoperablejs.googlecode.com/svn/trunk into >>>> mozilla/js/rhino/testsrc/interoperablejs. This way we can also do a >>>> changes in tests itself.
>>>> Let me know what do you think about it.
>>>> Regards, >>>> Jarek
>>>> W dniu 8 lutego 2010 08:59 użytkownik Attila Szegedi >>>> <szege...@gmail.com> napisał:
>>>>> Hi
>>>>> yes, please. I did start working on a JUnit driver for them myself, but if you already have it completed, I'll be happy to accept your patch and save me some work.
>>>>> Thanks, >>>>> Attila.
>>>>> On 2010.02.08., at 8:57, Jarosław Pałka wrote:
>>>>>> Attila,
>>>>>> I have finished tests with interoperablejs. I wonder how should I share it with you. Should I attach tests suite to bugzilla issue?
>>>>>> Jarek
>>>>>> W dniu 8 lutego 2010 08:14 użytkownik Attila Szegedi <szege...@gmail.com> napisał: >>>>>> On 2010.02.07., at 20:33, Jarosław Pałka wrote:
>>>>>>> Attila,
>>>>>>> I was playing with your patch for few days, and one thing I found is annoying NullPointerException when module is not found :), I believe we should have more meaningful exception message.
>>>>>> No, I haven't, but I definitely will now. I'm writing JUnit tests. I just recently discovered that there's an EMMA plugin for Eclipse, it greatly improves my progress towards good coverage. I have found few bugs myself.
>>>>>> Also, I have found a way to make Require thread-safe for use with shared top level scopes. And by "found a way" I mean "found a more efficient way than using a coarse synchronized block on it" :-)
>>>>>>> So far code looks good, >>>>>>> Regards, >>>>>>> Jarek
>>>>>>> W dniu 31 stycznia 2010 20:15 użytkownik Attila Szegedi <szege...@gmail.com> napisał: >>>>>>> Hi all,
>>>>>>> I just put out the second implementation patch at <https://bugzilla.mozilla.org/show_bug.cgi?id=540724>, against current CVS HEAD. Check it out if you're interested. I have worked on this for I most of my free time I can have for coding in the last 12 days, and have arrived at a much better design than what was in the first attempt. I attached a comment to the Bugzilla issue describing the design decisions I took. It all feels round to me at the moment. I took care to document all classes and interfaces in great detail. I'll proceed with writing tests for it, but if you don't mind reviewing and trying bleeding-edge stuff, go for it.
>>>>>>> Attila.
>>>>>>> On 2010.01.19., at 23:45, Attila Szegedi wrote:
>>>>>>>> Hi,
>>>>>>>> I created a Bugzilla issue to track this work: <https://bugzilla.mozilla.org/show_bug.cgi?id=540724> >>>>>>>> I already attached a patch with the implementation to the above issue, so feel free to check it out and provide feedback. I believe that the JavaDocs I provided are sufficiently comprehensive so that no one should have trouble understanding how's it used.
>>>>>>>> Be warned it's quite untested - I'll proceed with writing some tests tomorrow.
>>>>>>>>>> On Jan 17, 1:35 pm, Attila Szegedi <szege...@gmail.com> wrote: >>>>>>>>>>> Folks,
>>>>>>>>>>> I'm contemplating adding CommonJS Modules implementation to Rhino >>>>>>>>>> codebase proper. I'd create org.mozilla.javascript.commonjs package to hold >>>>>>>>>> it, and we could have a method similar to initStandardObjects(), i.e. >>>>>>>>>> initCommonJs() that'd initialize it - basically install a require() function >>>>>>>>>> with the expected semantics in the top-level scope. I want leave some of its >>>>>>>>>> aspects - most notably lookup of the module script - pluggable, defined by >>>>>>>>>> interfaces in the org.mozilla.javascript.commonjs package, so that specific >>>>>>>>>> embeddings of Rhino (JS app servers) can install their own module resolver >>>>>>>>>> logic. I'd provide a default implementation for the shell too.
>>>>>>>>>>> As I foresee that several Rhino-based JS products will adopt CommonJS in >>>>>>>>>> the near future, it seems desirable to not have all of them reinvent the >>>>>>>>>> wheel (even though some already did, I'm guilty of coding my own
Sorry Attila, I forgot I still owe you an answer...
On Feb 3, 6:39 pm, Attila Szegedi <szege...@gmail.com> wrote:
> On 2010.02.03., at 17:52, Hannes Wallnoefer wrote:
> > Other than that, I really like your implementation. Helma NG does a > > few things differently, so it's not likely we'll replace our > > implementation anytime soon, but I think I'll take some inspiration > > from your code for things it does better than ours.
> :-)
> Is there a way to reconcile the differences? I tried to follow Modules/1.1 to the letter... Also, maybe your implementation is doing something better than mine does, in which case it wouldn't be bad if I'd get some inspiration the other way :-)
I put a lot of work and thought into the design of Helma NG (which we're currently relaunching as RingoJS). As a result, it is a bit farther away from the "classical" JS shell, but there are some very tangible benefits.
For example, module scopes are top level scopes in HNG/RingoJS. This gives as complete module isolation even with a shared "classical" global scope. Also, the require() function in Helma NG is shared among multiple contexts and threads, and we're using a threadlocal to keep maps of loaded modules per cx/thread.
This gives us a lot of flexibility. For example, while modules are instantiated per context/thread by default, modules can opt to have one instance shared among all threads simply by setting a flag in their module meta-object, giving us optionally multithreaded modules. There's also completely transparent reloading of modules that takes care of module dependencies for shared modules.
To sum it up, HNG/RingoJS aims a bit farther away from the typical JS shell/runtime. That's why I think it should remain a dedicated project (I've also thought about proposing our module loader for inclusion in Rhino at certain times). But I also think your implementation makes perfect sense to be included in Rhino for more "conservative" CommonJS runtimes.
>> I've started playing with the patch in the context of my ESXX project and while I haven't had time to finish yet I have a couple of questions:
>> 1) I already have a path array in ESXX, which also happens to be a JS array that is searched from the front. However, having two path objects is weired, so I'm wondering if we could either perhaps replace the 'secure' argument with a 'paths' argument (Scriptable) so I can inject mine, or add a method that returns Require's paths object so I can use that one instead?
> You can easily obtain the paths of a require instance. Remember, a require() function is just a JS object, so you can do:
Right. The problem is that it turned out that my existing path array is not an array of Strings but an array of (my custom) URI objects, so I would need to provide a custom JS object that automatically presents a string "view" but at the same time allow modifications. However, even if I was to putProperty() that object onto Require(), the Require code will still use its internal reference.
Anyway, I ended up simply require.delete("paths") for now. From what I see, the paths property is optional according to CommonJS.
>> 2) I have a special property named 'esxx.location' that contains the URI of the currently executing file. For this to work I need to know when the Script is executed. It could be solved by making Require.executeModuleScript protected, or better, adding an exec method to ModuleScript.
> I see - you need a post-exec hook. Okay, that's a reasonable thing. I actually at one point in time had ModuleScript.exec(), but I scrapped it. The reason is in the way SoftCachingModuleScriptProvider is implemented; if you look into it, ModuleScript instances in it are not even preserved, but they are deconstructed/reconstructed into/from the ScriptReference as needed. This is done so that there's no strong reference Script objects being held, so unused scripts can indeed get unloaded. (functions have pointers to their parent scripts, so as long as there's any function from the script reachable in any of the currently running JS program instance, they won't get GCed, otherwise they can be). For the same reason, you shouldn't create such post-exec hook by creating a wrapper around Script that itself implements Script with pre/post processing - you'd screw up soft caching as you can't sneak in a strong reference to your wrapper Script object into the Script object that Rhino compiles for you, which'd be required to ensure soft cache doesn't load multiple reachable copies of scripts (& functions).
> So, an explicit way to register a post-exec hook is the cleanest solution - I'll look into where would this fit best.
Unfortunately, that didn't really help much, since
1. The code I'm interested in running is Java, not JS 2. No reference to the ModuleScript in question is passed to the pre/post hooks 3. I would need hold state using a custom stack in order to restore the location in the post-hook
I ended up with the following (somewhat hackish) ModuleScript implementation instead:
public class ESXXScript extends ModuleScript implements Script {
public ESXXScript(Script script, URI uri) { super(script, uri.toString()); this.uri = uri; }
@Override public Script getScript() { return this; }
@Override public Object exec(Context cx, Scriptable scope) { URI old_location = setCurrentLocation(uri);
However, I still think it would be better if ModuleScript extended Script and used public Object exec(Context cx, Scriptable scope) instead of public Script getScript(). I can't see how that would prevent soft-references from working as intended, but anyway, ESXXScript works for me and I'm happy with it as it is.
More importantly, however, is the installation of the main module. I'm setting up the application scope using custom code and not loading it as a module, so Require.requireMain() is not applicable here. Require.install() only installs the require() function, it does not set up a main module.
I modified Require() to include an installMain() method. Patch attached. Would you consider to include it? (The patch is against your 3rd Bugzilla patch; I'm on the Rhino 7r2 branch.)
