Need mechanism for multi-file JS components

RESOLVED FIXED

Status

()

Core
XPConnect
P3
minor
RESOLVED FIXED
18 years ago
13 years ago

People

(Reporter: Aaron Leventhal, Assigned: Robert Ginda)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

(Reporter)

Description

18 years ago
We need the ability to include, or import javascript from inside another 
javascript file.
To what context are you referring?  In the JS shell (and xpcshell/lcshell
variants), there is a ``load'' command.  Are you loading a script into content
with <script src=>?  A JS component?

The JS component case is a little trickier, because:

 - you have to detect file changes across all included files when you're
figuring out when to re-register (maybe not a problem, long term)

 - you have to specify the name of the included file somehow, which gets a
little tricky.  If you use an URL (or relative-URL) syntax, you have to have
necko stuff available when you're executing the component script, such as at
registration time.  If you use a relative path name, you're constraining
yourself to local files (perhaps not a big deal now; webapp stuff might make it
annoying later), and you have all sorts of XP-file-path issues to face.

I'm not sure what I think about this.  Doing ``import'' for JS components and
hoping that relative file paths work -- or waiting for rayw's search-path work
to land? -- is a little tempting, but I fear making a hash of things.

Comment 2

18 years ago
Much on this topic was already discussed in bug 14949
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Can't use import statement in Javascript → Need mechanism for multi-file JS components
Yeah, that one covers the in-chrome case pretty thoroughly, but I'm not sure the
issues are the same for JS components, so I'm re-titling this one and keeping it
open.
(Assignee)

Comment 4

18 years ago
I'm starting to need this in a bad way for chatzilla.  How about a component 
with an interface something like...

interface nsIScriptLoader {
  bool loadURL (in wstring url);
  bool loadFile (in nsIFile file);
};
Why a new interface (service too, I guess)?  If you can use nsILocalFile, 
nsIDirectoryService, and eval(), go for it.

/be

Comment 6

18 years ago
If it doesn't work with relative paths (url or local file) then it is broken. 
The last thing we want is people being forced to use absolute paths to load 
their library code.
I think I'd prefer a component-context API function, if only so that it could
eventually co-operate with the chrome script cache (or one of its own) to avoid
compiling multiple copies of the same script (IO.js or string-bundle.js, for
example).
(Reporter)

Comment 8

18 years ago
The question was put: what context am I using the scripts in?
When designing a large app that will utilize many javascript function 
libraries, it is better to list those dependencies within the source javascript 
file itself, then in each XUL file. I am not familar with the load() function.

Comment 9

18 years ago
Aaron, load() works in xpcshell. 

load('myLib.js');

xpcshell is part of debug builds. 

For now, having a simple load() top level method that automatically deals w/
file url, chrome, local and full path issues would be great.

But if no one has time, then i guess we can do what Brendan suggests.
Perhaps as a top level method w/ a simple API that will handle these details of
the various flavors of local file paths.

So then we can just do a:

include('myLib.js');

This method can also return if any dupe files are passed to it . . .

It probably wouldn't take very long to do.

pete

(Assignee)

Comment 10

18 years ago
Created attachment 13329 [details]
iotest.js; runtime js file inclusion, implemented in js
(Assignee)

Comment 11

18 years ago
Created attachment 13330 [details] [diff] [review]
xpc.diff; Components.load, implementd in C++
(Assignee)

Comment 12

18 years ago
Here's two whacks at a solution...

iotest.js doesn't associate a filename with the new script, so error messages
would be pretty hard to decipher.  Also, you'd have to cut n' paste this into
the first js file that needed to include another js file.

xpc.diff adds wacky dependancies to xpconnect, check out the new #includes in
xpccomponents.cpp (nsIResChannel.h is bogus, I forgot to remove it from the
diff), but you get better error reports.  Also, we should probably set some
principals on the new script, and throw compile time errors back to the caller,
instead of returning NS_ERROR_FAILURE.

Comment 13

18 years ago
Rob, i couldn't get your first example to work.
I added an include function to io.js w/ error checking, etc . . .

However, eval is not working. Anyone know what i am doing wrong here?

Attaching now . . .

pete

Comment 14

18 years ago
Created attachment 13414 [details] [diff] [review]
DIFF for io.js

Comment 15

18 years ago
Here is my test:

js> 
load('/usr/src/THEME_BUILDER/Theme_Builder/theme_builder/theme_builder/content/js/io.js');
js> var file = new File();
js>
file.include('/usr/src/THEME_BUILDER/Theme_Builder/theme_builder/theme_builder/content/js/test.js');
function pete() { dump("\n\n\n\n********** TEST ***************\n\n\n\n"); }
js> pete();
typein:4: ReferenceError: pete is not defined

Comment 16

18 years ago
Rob, here is your code error.
I just pasted it in . . . 

----------------------------------------------------

js> const IOS_CID = "{9ac9e770-18bc-11d3-9337-00104ba0fd40}";
js> const nsIIOService = Components.interfaces.nsIIOService;
js> const SCRIPTABLEIS_PROGID = "component://netscape/scriptableinputstream";
js> const nsIScriptableInputStream =
Components.interfaces.nsIScriptableInputStream;
js>
js> function loadURL (url)
{
    var ios = Components.classesByID[IOS_CID].createInstance(nsIIOService);
    var chan = ios.newChannel (url, null);
    var is = chan.openInputStream();
    var sis =
Components.classes[SCRIPTABLEIS_PROGID].createInstance(nsIScriptableInputStream);
    sis.init (is);
    var script = sis.read (chan.contentLength);
    return eval(script);
}
js>
loadURL('/usr/src/THEME_BUILDER/Theme_Builder/theme_builder/theme_builder/content/js/test.js');
uncaught exception: [Exception... "Component returned failure code: 0x804b000a
[nsIIOService.newChannel]"  nsresult: "0x804b000a (<unknown>)"  location: "JS
frame :: typein :: loadURL :: line 9"  data: no]
js>

Comment 17

18 years ago
the onbly way i can get this to work is by doing this:


js> load('/usr/src/M18/mozilla/xpcom/tests/utils/io.js');
js>  var file = new File();
js>
eval(file.include('/usr/src/THEME_BUILDER/Theme_Builder/theme_builder/theme_builder/content/js/test.js'));
js> pete();
********** TEST ***************js>
js> 


Anyone know why returning an eval doesn't behave as would be expected?


(Assignee)

Comment 18

18 years ago
Sorry, I guess I should have specified that these routines load file:,
resource:, and chrome: urls ONLY.  If you prefix your path with "file://" it
should work as expected.  Though, I don't think you'll be happy about it.  Any
errors generated by the included script will be quite hard to track down.

On another note, I spoke with jband about xpc.diff, and since he's too shy to
make his comments here, I'll try to paraphrase...

1) The new dependencies are bad, he'd prefer that we only create the load()
method if the required components exist.

2) jband is morally opposed to checking in anything that can't do relative
paths.  He doesn't consider resource: and chrome: as meeting this criteria.

RE point 1, it sounds like we'll end up creating a script-loader component
anyway, so why not just component-ize this code, and leave it to the script
writer to instantiate the component.

RE point 2, IMO, this is a case for Worse is Better, but I'm biased.  I'd love
to have this for chatzilla.  Relative paths could be added to the component at a
later time w/o affecting current users, who would be using fully qualified
urls.  Also, in the context of mozilla, chrome: and resource: urls are relative
enough for most purposes (like, mine for example.)  Relative http: paths are
moot, as this routine doesn't load from http:.
If we're willing to apply Allman's law one more time, I think we can avoid the
dependencies without requiring (smaller, granted) boilerplate JS
copied-and-pasted everywhere.

How about the Load::call method tries to load a subscript-loader component by
progID the first time it's called, and then calls through to it if it's able. 
If it's not able to create the subscript-loader, it just bails with
NS_ERROR_UNAVAILABLE or whatever.

Our subscript-loader component would be part of the component loader, but only
built/installed ifndef XPCONNECT_STANDALONE, which means that it could also do
the privilege management.  Whaddya think?

Hrm.  The privilege management thing is actually a little scary in that model. 
Maybe the loader should just, ifndef XPCONNECT_STANDALONE, annotate the
Components object with the load() thingy.  The loader is already lightly sprayed
with XPCONNECT_STANDALONE conditionals, so what's one more?

Whaddya think?

Comment 20

18 years ago
For the here and now . . 

this is what i have for io.js. It will handle local file paths.
It registers included files so you don't use them again etc. . . 

However like i said, i can't get eval to work properly. So i am currently just
retuning the read as a string and using eval from the caller . . .

/********************* INCLUDE **************************/
include : function (path) {
  if(!path) {
  dump("missing argument\n\n");
  return;
  }
  if(this.leaf(path)=="io.js"){
  dump("Sorry, can't include io.js because you are already using it!\n\n");
  return;
  }
  if(!this.exists(path)) {
  dump("invalid path\n\n");
  return
  }
  if(this.includeFiles) {
    for(var i=0;i<this.includeFiles.length;i++){
      if(path==this.includeFiles[i]){
      dump("Already using this include file, ignoring . . .\n\n");
      return;
      }
    }
  }
  this.includeFiles.push(path);
  var retval;
  this.open(path)
  retval    = this.read(path);
  return retval;
},
/********************* INCLUDE **************************/

Comment 21

18 years ago
Wouldn't it be better to just use the DOM to do a HTTP GET, and then use an eval
() nested in a try{}?

Comment 22

18 years ago
Hmm...i misuderstood, you want to load resource: and chrome: .Ignore previous 
comment.
(Assignee)

Comment 23

18 years ago
So, if we're going to #ifdef XPCONNECT_STANDALONE, why make a new component when
we can just #ifdef out the body of the function (and the includes) and replace
it with a NS_ERROR_UNAVAILABLE.  Jband seems to think this would be ok, although
he still thinks including necho stuff in xpconnect is unpure (He half-jokingly
sugested it be a component living in the js-component-loader .so)

Anyway, I'll rework the patch to include the #ifdefs, walk the scope chain
instead of using JS_GetGlobalObject(), and report failures as js exceptions
instead of XPCOM failures, but I'll need help with the principals stuff
(shaver?)
Why was jband half-joking?  Let's make the JS component loader put its Load
object in place on the Components object when it bootstraps XPConnect.  Localize
the dependencies, etc.

The principal stuff is the other reason; what principal would you give something
that gets Components.load()ed?  The principal from the calling script?  Let's
make the component loader put that policy in place for its scripts, and maybe
save ourselves a firedrill later on.
I agree with jband (not joking) and shaver (i think, hope I'm following this all
accurately): we should make a separate component and not wire up necko into a
ball of string with XPConnect.

/be
(Assignee)

Comment 26

18 years ago
Making the js component loader strap the load() function to the Components
object (e.g. with JS_DefineFunction) wouldn't expose the load() function to
chrome.  This may or may not be a good thing.  I'd probably argue the latter.

Turning the code into an nsIXPCScriptable component living the the js component
loader module WFM.  XPConnect can attempt to instantiate the component (by
progID, as shaver said) in nsXPCComponents::GetLoad(), and throw an error if
that instantiation fails. 

comments?
(Assignee)

Comment 27

18 years ago
Created attachment 13469 [details] [diff] [review]
xpc.diff; second attempt
(Assignee)

Comment 28

18 years ago
Created attachment 13470 [details]
js/src/xpconnect/loader/mozJSSubScriptLoad.cpp
(Assignee)

Comment 29

18 years ago
Created attachment 13471 [details]
js/src/xpconnect/loader/mozJSSubScriptLoad.h
(Assignee)

Comment 30

18 years ago
I used the CLSID instead of the progID, but the rest is my interpretation of
what we've discussed.
I don't care about solving the exposure-to-chrome issue right now.

I chose ``progID'' on purpose, because it would allow XPCONNECT_STANDALONE users
to provide their own load() implementation, which seems like a nice thing.

I'll review your patch now...
(Assignee)

Comment 32

18 years ago
ah.  Good point.  I've changed it to a progID here.
What else?  (the new script still has no principals.)

Comment 33

18 years ago
A few things...

- The reason I was on;ly half serious about putting this code in the jsloader 
was because it didn't seem that much better to saddle the loader with this necko 
dependency than to saddle xpconnect. But, everyone seems to be fine with this. 
whatever.

- Like rginda mentioned, he still needs some principals to compile his script. 
This reminds me of somethng that bothers me a lot about this. He supports 
passing in a scope object for the compilation. But, we've given JS loader 
objects system principals. This means (I think) that if one has the specific 
transient privileges to both get access to some JS loader loaded object (e.g. 
window.sidebar that *everyone* can get) and to call Components.load, then one 
can load code that will end up with system principals and which will never be 
checked for privileges at all. This seems broken to me.

- I'm not real clear on why this is Components.anything - i.e. what does a JS 
file loader have to do with Components? Why not require JS code to 
getService for the loader service and then call 'load'. This allows you to get 
rid of the nsIXPCScriptable junk (for which you've added no security check BTW).

- again, I think it sucks that we might leave this as a way to load fully 
qualified urls without any mechanism to do relative paths (which take into 
account the page (and/or! location of the .js file) from which they are called. 
This is in large part a mechanism to allow library code to load other library 
code. Needing to know absolute urls (even if they are remapped chrome: urls) 
sucks. But, I bend to worse is better arguments as necessary.

Anyway, what is so bad about having people write:

function load(url, obj) {
  // XXX get privs here if required
  var iface = Components.interfaces.nsIJSSubScriptLoader;
  var clazz = Components.classes['whatever'];
  var loader = clazz.getService(iface);
  return loader.load(url,obj);
}

The service implementation can use nsIXPCCallContext to deal with optional 
JSObject param and to get at the JSContext. The security check on whether or not 
hte user is allowed to call this particular method of this interface is done 
automtically.

Yes, this is more JS code that users would have to write. Is that so bad?

And then... Why not have window.load ? Why should xpconnect privileges come into 
play? Shouldn't this really be about a codebase check; i.e. this file can load 
that file or not.

I realize that Rob wants to get something working so that he can get on with his 
work. But I really don't want to see us institute something that we'll regret.
Group: netscapeconfidential?

Comment 34

18 years ago
Make that 'window.loader' and it can implement 'load' and whatever else we're 
likely to think of in the future. The JS Component loader can have its own 
'loader' object hanging off its global. We can add security checks (somehow?) on 
the parent object that gets passed in and decide whether or not we want to allow 
it to be used as the parent for the code to be compiled. We could even preclude 
passing an explicit parent object when called from content JS.
(Assignee)

Comment 35

18 years ago
I've reworked this to be a standalone component, included in the jsloader
binary, and using nsIXPCCallContext instead of the scriptable stuff.  I'm going
to use it as part of a project I'm playing with and see how it all "feels", I'll
post new patches when it's a little more shaken out.
Depends on: 50447
rginda's gonna do this, it seems.  I'm just going to watch. =)
Assignee: shaver → rginda
Status: ASSIGNED → NEW
(Assignee)

Comment 37

18 years ago
Accepted bug.  Why was this set to "nscp confidential?"... unsetting.
Group: netscapeconfidential?
Status: NEW → ASSIGNED

Comment 38

18 years ago
Rob, It looks like I toggled that without noticing. I've inadvertanly tabbed to 
that box before without catching it right away. I guess I did this here without 
noticing at all.
(Reporter)

Comment 39

18 years ago
Quick crude way that works
--------------------------
=> Can it be simplified further?

function jsInclude(path)
{
  try {
    var fileInst                = new FilePath(path);
    var fileChannel             = new FileChannel();
    var inputStream             = new InputStream();
    fileChannel.init(fileInst, 0x01, fileInst.permissions);  /* Open to read */
    var inStream = fileChannel.openInputStream();
    inputStream.init(inStream);
    eval(inputStream.read(fileInst.fileSize));
    inStream.close();
    }
  catch (e) {
    dump("jsInclude ERROR: "+e+"\n\n");
    }
}

jsInclude("includethis1.js");

(Reporter)

Comment 40

18 years ago
Oops - forgot to set these consts at the top.
Here's the whole simple solution again.

/////////////////////////////////////////////////////////////////////////
const FilePath    = new Components.Constructor( "@mozilla.org/file/local;1",
"nsILocalFile", "initWithPath");
const FileChannel = new Components.Constructor(
"@mozilla.org/network/local-file-channel;1", "nsIFileChannel" );
const InputStream = new Components.Constructor(
"@mozilla.org/scriptableinputstream;1", "nsIScriptableInputStream" );

function jsInclude(path)
{
  try {
    var fileInst                = new FilePath(path);
    var fileChannel             = new FileChannel();
    var inputStream             = new InputStream();
    fileChannel.init(fileInst, 0x01, fileInst.permissions);  /* Open for reading */
    var inStream = fileChannel.openInputStream();
    inputStream.init(inStream);
    eval(inputStream.read(fileInst.fileSize));
    inStream.close();
    }
  catch (e) {
    dump("jsInclude ERROR: "+e+"\n\n");
    }
}

jsInclude("/home/aaron/fred.js");
(Reporter)

Comment 41

18 years ago
Sorry for the bugspam, one more correction:
Make jsInclude return a string of what you want to eval outside of jsInclude.
So you'll end up doing:
eval(jsInclude("include-me.js"));

Otherwise all of your functions and consts you define will be destroyed after
jsInclude returns, because of scope.
(Reporter)

Comment 42

18 years ago
I'm having 2 problems with the read-file/eval it method just derscribed:
1. Javascript warnings and error messages give the wrong file and line numbers,
so problems are much harder to track down.
2. I have the main component in dist/bin/components, but where should I store
the include files? I don't want them in the same directory, otherwise they get
initialized on startup.
(Reporter)

Comment 43

18 years ago
Solution to file location.
As suggested by John Bandauer, I named the include files with a .jsl extension
(javascript library file) and stuck them in the components directory along with
the .js component that uses them.
I also had to change the js code that includes them to now find the proper
directory that they're in.
We still have the problem with the line numbers being incorrect in error/warning
messages.

Here's the complete code, that includes files from the components directory:

////////////////////////////////////////////////////////////////////////////////////////////
//  Include javascript files - this works until a better multi-file js mechanism
is achieved
/////////////////////////////////////////////////////////////////////////////////////////////

// First get include directory name
var dirServiceProvider =
Components.classes["@mozilla.org/file/directory_service;1"].
   getService().QueryInterface(Components.interfaces.nsIDirectoryServiceProvider);
var persistent = new Object();
var includeDir = dirServiceProvider.getFile("ComsD",  persistent).path;

// This returns the include file from the include directory as a string
function jsInclude(includeFileName)
{
  var path=includeDir+"/"+includeFileName;
  const FilePath    = new Components.Constructor( "@mozilla.org/file/local;1",
"nsILocalFile", "initWithPath");
  const FileChannel = new Components.Constructor(
"@mozilla.org/network/local-file-channel;1", "nsIFileChannel" );
  const InputStream = new Components.Constructor(
"@mozilla.org/scriptableinputstream;1", "nsIScriptableInputStream" );
  try {
    var fileInst                = new FilePath(path);
    var fileChannel             = new FileChannel();
    var inputStream             = new InputStream();

    fileInst.permissions=0x0755;
    const READ_MODE=0x01;
    fileChannel.init(fileInst, READ_MODE, fileInst.permissions);  /* Open for
reading */
    var inStream = fileChannel.openInputStream();
    inputStream.init(inStream);
    var ret=inputStream.read(fileInst.fileSize);
    inStream.close();
    }
  catch (e) {
    dump("jsInclude ERROR: "+e+"\n\n");
    return "";
    }
  return ret;
}

// Now go get the files and eval them
eval(jsInclude("nsAccessibleState.jsl"));
eval(jsInclude("nsAccessibleCommands.jsl"));

//////////////////////////////////////////////////////////////////////////
(Assignee)

Comment 44

17 years ago
It's been a while, but better late than never, I guess.

Attached after this comment is a patch that adds a new component,
"@mozilla.org/moz/jssubscript-loader;1".  The component lives in the
js-component-loader library.  It has a single method, loadSubScript(), that
takes as parameters a URL to load, and optionally (through the magic of
nsIXPCNativeCallContext) an object to use as a global during the eval.  The URL
*must* refer to a local file, and use a file:, resource:, or chrome: scheme. 
This component is only intended to be called from JavaScript code.

Loaded scripts are given the system principals, and no caching of compiled
scripts is done.

Because the loader adds dependencies on nsIChannel and nsIIOService, it can be
removed from the build by #define-ing NO_SUBSCRIPT_LOADER or XPCONNECT_STANDALONE.

Attached is the main file, mozJSSubScriptLoader.cpp for online review, and a
.zip of the entire mozilla/js/src/xpconnect/loader subdirectory for more
complete review and testing.

Mac build goop still needs to be added, I'll need some help there.

looking for some hot s?r= action.
(Assignee)

Comment 45

17 years ago
Created attachment 26652 [details]
mozJSComponentLoader.cpp
(Assignee)

Comment 46

17 years ago
Created attachment 26653 [details]
loader.zip, the entire mozilla/js/src/xpconnect/loader directory
(Assignee)

Comment 47

17 years ago
um, oops.  I attached the wrong .cpp file, here comes the right one.
(Assignee)

Comment 48

17 years ago
Created attachment 26655 [details]
mozJSSubScriptLoader.cpp
I dig it with a vengeance, and want it for 0.8.1.  sr=shaver, let's get some r=
and slip this baby in.
(Assignee)

Comment 50

17 years ago
attaching updated mozJSSubScriptLoader.cpp, fixes include:

NS_WITH_SERVICE->do_GetService changes
text for null |buf|

In addition, I'll be chkecing the idl file into xpconnect/idl, instead of
xpconnect/loader/idl.

The 0.8.1 deadline is today!  Still looking for a r=, jband, brendan, bueller?

adding peterv to cc list for mac build mods.
(Assignee)

Comment 51

17 years ago
Created attachment 27640 [details]
updated mozJSSubScriptLoader.cpp
(Assignee)

Comment 52

17 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

13 years ago
Component: XP Miscellany → XPConnect
You need to log in before you can comment on or make changes to this bug.