On 2010.02.03., at 17:52, Hannes Wallnoefer wrote: > On Jan 31, 8:15 pm, Attila Szegedi <[email protected]> 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, > > I just had a look at your patch, here are some quick impressions.
First of all, thank you very much for taking the time to review it. > The general code layout seems clear and well thought out. Thanks; I have the habit of obsessing over details before I release something; I significantly transformed the damn thing several times before I was satisfied that I dare show it to other people :-) > The only > problem I had is the similarity between ModuleSource and ModuleScript > and related interfaces and classes. *ModuleSourceProvider and > *ModuleScriptProvder just look very similar, and it doesn't help that > both live in the same package. I know the naming makes sense, but > maybe it's possible to find terms that are farther apart. In Helma NG, > we use the terms Repository and Resource for module source > interfaces. Well, I'm open to that if better suggestions come up for my current terms "module source" and "module script" - the former represents source code, the latter represents a compiled object (actually holds an instance of org.mozilla.javascript.Script). I attribute high significance to the identifiers I use - code legibility is a very important aspect for me; I did rename many of those classes several times before I dared to put the patch out :-). I.e. "ModuleScript" used to be just "Module", but then I realized that in this terminology, "module" is a JS object, an instance particular to an execution of the program, hence the class I had there couldn't be named "Module"... you can see how it goes. So anyway, what I wanted to say is, I'm open to better naming suggestions. I don't like "resource" probably because I'm indoctrinated with REST beyond hope of recovery - any particular source text retrieved through an URL is at best "a text entity that is the representation of the resource at the time of retrieval". See, I told you I'm beyond hope. I was considering "ModuleSourceEntity", but then settled for "ModuleSource" :-) > On the source provider side, I'm not sure the default implementation > should be based on URI/URLs. I think local, file based module > repositories will be more common than HTTP for CommonJS Rhino, and > while URLs cover all this, they just don't have an easy validation > mechanism for local files. Well, there's File.lastModified(); we have to settle for it... As a matter of fact, I'm still thinking that I would need to force non-caching of any resource where last-modified is less than two seconds before the retrieval, to make sure we don't miss multiple updates within a single second with an underlying resource provider that only provides one-second resolution (most filesystems are better, but HTTP doesn't go below one-second resolution, and indeed HTTP/1.1 spec explicitly recommends this practice for caching). > At least I think there should be a > FileModuleSourceProvider in addition to the UrlModuleSourceProvider. Possibly - my thinking was that using the UrlModuleSourceProvider with file: URLs yields the same results as a dedicated java.io.File-based provider would, and there's less code to maintain (I *love* cutting down on code; "functionality is asset, code is liability" is my mantra). Using files through file: URLs is not noticeably less efficient than using java.io.File API directly. Granted, it'll set an If-Modified-Since header in vain on revalidation checks, and there'll be one gratuitous open/close of the file involved, but that's pretty much the only additional overhead (and this only happens once per expiry duration, cca, every 60 seconds by default). Of course, a "FileModuleSourceProvider extends ModuleSourceProviderBase" would indeed be easy to implement. > 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 :-) Attila. > Hannes > >> 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. >> >>> Attila. >> >>> -- >>> home:http://www.szegedi.org >>> twitter:http://twitter.com/szegedi >>> weblog:http://constc.blogspot.com >> >>> On 2010.01.18., at 14:54, Jaros³aw Pa³ka wrote: >> >>>> Count me in as well. >> >>>> Jarek >> >>>> 2010/1/18 Rapha <[email protected]> >> >>>>> I like the idea. Are you thinking of the 1.1 spec ( >>>>> http://wiki.commonjs.org/wiki/Modules/1.1) ? >> >>>>> Raphael >> >>>>> On Jan 17, 1:35 pm, Attila Szegedi <[email protected]> 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 require() >>>>> too in the next-gen version of my company's server-side JS enviroment...). >> >>>>>> Opinions? >> >>>>>> Attila. _______________________________________________ dev-tech-js-engine-rhino mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-js-engine-rhino
