628669 - Provide support for relative URLs in Components utils import (JSM, JS modules)

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jonathan Protzenko [:protz]]

Rationale: I'm currently writing a thunderbird-stdlib project which is basically a collection of JSMs. Extension authors are encouraged to checkout a copy of this into their tree, so that it appears as resource://theirext/stdlib/. Problem is, module A from stdlib depends on module B from stdlib too.

Possible non-solutions include:
- hardcode resource://theirext/ into stdlib's modules, which requires other extension authors to modify the stdlib, and makes their lives hell because I encourage the gitsubmodule way of checking it out, so that makes a difference with -master
- write resource://__IMPORT_PREFIX__/stdlib/ everywhere in stdlib, and package it with a shell script that automates the process (basically a whole bunch of find and sed's)
- use the magic __LOCATION__ and make a wild guess, from the extension id ([e-mail]), that the path will be resource://xxx/stdlib/ (and require the extension to be unpacked so that __LOCATION__ be defined.

All of these are non-satisfactory, so I think relative URLS should be accepted, i.e. I should be able to write:

Components.utils.import("./moduleB.js")

from moduleA.js

Any comments, thoughts and/or guidance will be highly appreciated, as I plan on tackling this as soon as I can find some time.

<ted> http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1412
<ted> is the guts of C.u.import
<ted> FWIW

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: Add a magic __URI__ global in JSMs

So this is a middle-ground solution, in the sense that it implements a magic __URI__ global, just like __LOCATION__, in JSMs. This allows JSMs to know which URI they're being imported from, which would solve my problem.

Of course, the relative URI solution seems better in the long run, so I have little hope that this patch will be accepted. I'm submitting this patch anyway for the following reasons:
- to be honest, this is not my area of expertise, and the patch above fit nicely in the one-hour timespan that I was willing to devote to this issue =) (this includes firing up all the mozilla string guide pages on MDC!),
- now a more serious reason, I believe this is very low-risk, which gives it an epsilon chance of being accepted before Firefox 4 (although I'm only interested in seeing this happen in Thunderbird, I still do believe it will be useful for other people)
- doing the relative import thing would make it absolutely not acceptable to land at this point of the release cycle,
- I'm willing to spend more time to add support for relative URIs in Components.utils.import, maybe for Firefox 5.

Although my xpcshell testing shows that redefining a var __URI__ global doesn't cause any warning, it might be that xpcshell ate them away. But this won't break any existing JSMs.

Andreas, I've requested review from you as you're listed as one of the peers for this module. Please do change the review request to someone else if you're not the best person to review this. Thanks! :-)

Any thoughts/comments are most welcome.

Comment 2

[Andreas Gal :gal]

Comment on attachment 507877 [details] [diff] [review]
Add a magic __URI__ global in JSMs

For API changes lets get the module owner's opinion.

Comment 3

[Jonathan Protzenko [:protz]]

Ok. One thought that just crossed my mind: we could also add a ImportRelative function in XPCOMUtils or whatever. One would use it as follows:

ImportRelative(this, __URI__, "otherModuleThat'sInTheSameDirectory.jsm");

That function would do the messy job of parsing __URI__ (probably through a nsIFile) and replacing the leafName with the third argument before important all of the functions from the module into the first argument.

But let's not anticipate and wait for Blake's opinion about this =).

Comment 4

[Jonathan Protzenko [:protz]]

Attachment: New version that uses the JSAPI in a proper way

New version of the patch. The string is now attached onto the global object through JS_NewStringCopyN and JS_DefineProperty. I also added more tests, and a utility function in XPCOMUtils that will make sure users of this functionality won't have to rewrite the lastIndexOf stuff everytime they wish to do a relative module import.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

bsmedberg, would you be OK with something like this going into FF4?

Comment 6

[Benjamin Smedberg]

So we're exposing __URI__ as a string, not a nsIURI? I think I'm ok with that. The actual importRelative function does some funky string-fu to create a URI, though: I wonder if we can't use something more complete...

I really don't think this is worth working on now, though.

Comment 7

[Jonathan Protzenko [:protz]]

bsmedberg, would it be better for you if the actual importRelative function interpreted __URI__ as a nsIUri to make sure it is well-formed? I think I can upload a new patch that does that if that's the kind of modifications you would like to see before approving this.

I'm not trying to make the Firefox 4 deadline, but rather the Thunderbird 3.3 alpha3 one which should be further in the future. I do have time to devote to this issue, so as long as someone is ok reviewing this (rather small) patch, I can keep working on this :-).

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 510981 [details] [diff] [review]
New version that uses the JSAPI in a proper way

I'm OK with this. bsmedberg, any comments?

Comment 9

[Jonathan Protzenko [:protz]]

bsmedberg, it's been two months since you commented on this bug — there's 20 lines of code, and less than 20 lines of test. Any chance you could give your opinion? I did not expect this to make it for Firefox 4, but I did not expect it to miss the Firefox 5 deadline either :-).

Comment 10

[Benjamin Smedberg]

Comment on attachment 510981 [details] [diff] [review]
New version that uses the JSAPI in a proper way

Does this work with ".." paths, or does that confuse the xpconnect module cache?

Comment 11

[Jonathan Protzenko [:protz]]

Attachment: With a slightly more involved test

Thanks for the feedback. So it looks like the xpconnect module cache is not confused when I use a ".." path, but there's no reason why it should, since I'm falling back to a very standard call to Component.utils.import.

I've added a alternative version of the patch that has a slightly more involved test, that checks that two imported submodules (the same submodule, but with two different paths) are pointing to the same scope with the same references, i.e. they're not instantiated twice.

I don't really think this is necessary, but if that makes you feel better, I can definitely make the importRelative function I introduced use a nsIURI and its spec property, though I don't think it's worth the overhead.

Comment 12

[Benjamin Smedberg]

Comment on attachment 525764 [details] [diff] [review]
With a slightly more involved test

Yes, the test allays my fears!

Comment 13

[Jonathan Protzenko [:protz]]

checked-in http://hg.mozilla.org/mozilla-central/rev/d223347c8cb7 !

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/JavaScript_code_modules/XPCOMUtils.jsm#importRelative%28%29

And mentioned on Firefox 6 for developers.

Comment 15

[A. Sel.]

Question: why is this in XPCOMUtils.jsm and not implemented as Components.utils.importRelative or even better implemented in Cu.import itself to differ between URLs and relative paths?

Is this by design or was it just the fastest way to implement for a lower priority feature request?

Comment 16

[Jonathan Protzenko [:protz]]

Your suggestion is indeed much better. I guess I just wasn't skilled enough to implement it as you suggest at the time, and maybe the reviewer preferred a non-invasive change. I believe the feature is useful in its current state, but In any case I'd be happy to see this implemented in a better way :)