Closed Bug 725412 Opened 14 years ago Closed 14 years ago

Add-on SDK should be able to use .jsm JavaScript Modules as CommonJS modules

Categories

(Add-on SDK Graveyard :: General, defect)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: zer0, Unassigned)

Details

Add-on SDK should have the capability to use JavaScript Module as CommonJS module: we can get benefit from a lot of modules are already around (e.g Thunderbird stdlib), and we also get some useful objects for free (e.g. Worker, ChromeWorker). If documented and described correctly to the developers, that functionality should help them for the transition from XUL / JS Module development to Add-on SDK. See: https://jetpack.etherpad.mozilla.org/require-jsm
Summary: Add-on SDK should be able to use JavaScript Modules as CommonJS modules → Add-on SDK should be able to use .jsm JavaScript Modules as CommonJS modules
OS: Mac OS X → All
Hardware: x86 → All
Brian, what is your take on doing this?
Whiteboard: [triage:followup]
If I remember correctly, BenB pinged us on irc about JSM support in Jetpack. It may be valuable to have his thoughts on this subject, isn't? I already gave my feedback to matteo during FOSDEM, It doesn't simplifying things to include JSM support in `require` statement. Supporting JSM looks like a low-level feature for porting old xul addons to jetpack. I tend to think that people *already* using JSM are willing to use Cu.import and would prefer `import` features/behavior more than `require` one. It doesn't make lots of value to force JSM to behave like CommonJS module. We would better explain and document how to convert a JSM to CommonJS and just use regular `require`. I can see value for a small helper module, but it really doesn't look usefull enough to complexify loader and/or add a new global for this purpose (and complexify documentation about `require`/modules). I think we are more missing tutorials on how to port XUL addons to Jetpack, than such ease of using XUL-things in jetpack.
This proposal was born from FOSDEM experience with Thunderbird developers, not from a comment in IRC, as I wrote on the MoPad document. The effort to implement this feature is minimal, and having people that start to use JS Module as CommonJS module will helps the transition phase. Also, `Cu.import` is not easy to detect on Python side, so it give benefit to us as well. More data we can provide to the add-on reviewer, better is. I don't see how have this such of helper for the transition phase will impact on having more tutorial and documentation: they're not mutual exclusive. To me, it simplifying the tutorials as well.
Importing a third-party JSM in a Jetpack module is trivial: const { Cu } = require("chrome"); /** * Imports a JS Module using Components.utils.import() * and returns the scope, similar to Jetpack require(). * * @param targetScope {Object} (optional) * If null, the scope will just be returned * and *not* added to the global scope. * If given, all functions/objects from the JSM will be * imported directly in |targetScope|, so that you * can do e.g. * requireJSM("url", this) * someFuncFromJSM(); * which will have the same effect as * Components.utils.import("url"); * someFuncFromJSM(); * in normal Mozilla code, but the latter won't work in Jetpack code. */ function requireJSM(url, targetScope) { var scope = targetScope ? targetScope : {}; return Cu.import(url, scope); return scope; } I think adding such a function makes sense, but it's only useful for third-party modules that you cannot modify. If the JSM is your own code, then 1) it's trivial to change EXPORTED_SYMBOLS to exports, and add require("chrome"). Most actual code can stay unchanged. 2) if your JSM in turn imports other JSM from you, you will have to edit them anyway, because your URLs changed when switching to Jetpack, so you can just as well change Cu.import() to require() and EXPORTED_SYMBOLS to exports.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3) > I don't see how have this such of helper for the transition phase will > impact on having more tutorial and documentation: they're not mutual > exclusive. To me, it simplifying the tutorials as well. I'm not saying that we shouldn't do any helper. But a simple helper *module* (not a global, nor require hack) would be sufficient, like the one described by Ben. I do not see enough value to move JSM from data/ to lib/ in order to justify specific code in loader for JSM (and specific documentation in require docs for JSMs). Do note that people already knows Components.utils.import so whatever you provide, they will have to learn yet another syntaxic sugar in order to use JSM, whereas we can simply document how to use Cu.import in jetpack world.
Would it make sense to add requireJSM() as function to the "chrome" module? Because if you have access to requireJSM(), you escaped the sandbox.
(In reply to Ben Bucksch (:BenB) from comment #6) > Would it make sense to add requireJSM() as function to the "chrome" module? > Because if you have access to requireJSM(), you escaped the sandbox. This (or a similar) approach works for me. In my mind the goal is to be able to use jsm modules *without* needing to require chrome, which has add-on review implications.
(In reply to Alexandre Poirot (:ochameau) from comment #5) > I'm not saying that we shouldn't do any helper. But a simple helper *module* > (not a global, nor require hack) would be sufficient, like the one described > by Ben. I don't actually interested in the technical implementation aspect, if it's part of the loader or if it's an helper in api-utils, as soon as: 1. You can "require" JS module as CommonJS module (so EXPORTED_SYMBOLS as actually mapped to `exports` object, you shouldn't specify any target object, otherwise you can just use `Cu.import`, this is to makes the JSM more like CommonJS module). 2. The JS Module will stay in the /lib folder rather than /data folder – *it's* a library, it's not part of the content or assets, separation *is* important. 3. The JS Modules don't messed up with any scope when required/imported 4. It's easily detected from the python tool as for `require("chrome")` so it can detect the type of access (load relative or absolute uri, for example). The last point is really important from our analysis and for add-on reviews. Even if it's true that require a JS Module is powerful as require("chrome"), if we have a specific syntax to detect the use of JS Module, we have more specific data – developer doesn't require general chrome access, but loading a specific JS Module, that if it's commonly used – e.g. Thunderbird stdlib – we can easily mark them and makes the review process easier. In any case, to me, it's definitely worthy have more data like that. Given that, I don't think it could be just an external utility module, but should be considered similar to `chrome` one – it has similar kind of "superpower" as well.
Matteo, what is your usecase (i.e. why you need it), not the solution (i.e. what you think you need)? Please see comment 4. I can see a usecase where you want to load JSMs that are part of the platform (toolkit, Thunderbird). I do not see a usecase where you ship the JSM in your Jetpack module. The conversion is trivial (I did it myself for many modules, it's really just a few lines) and in many cases changes are necessary anyways (even if you were using JSMs) due to the URL changes. In most cases, the following is enough to turn a JSM module into a Jetpack module: const { Ci, Cc, Cu, Cr } = require("chrome"); for each (let symbolName in EXPORTED_SYMBOLS) exports[symbolName] = this[symbolName]; > *without* needing to require chrome > The last point is really important from our analysis and for add-on reviews. But as I said, this is true anyway. If a Jetpack addon uses JSM modules, that's the same as having require("chrome") access. And you can not just take any arbitrary JS module, convert it to a Jetpack module, and then assume it's safe to expose. The API has to be written with that in mind, and JSM APIs are not. They assume the caller has chrome rights anyway and therefore they don't protect themselves against hostile callers.
> in many cases changes are necessary anyways (even if you were using JSMs) > due to the URL changes. In most cases, the following is enough ... (That is, apart from above mentioned URL changes - JSM itself using other JSM module, stringbundle loading properties files -, which you'll need in any case.)
(In reply to Ben Bucksch (:BenB) from comment #9) > Matteo, what is your usecase (i.e. why you need it), not the solution (i.e. > what you think you need)? > > Please see comment 4. I can see a usecase where you want to load JSMs that > are part of the platform (toolkit, Thunderbird). > > I do not see a usecase where you ship the JSM in your Jetpack module. The specific use case we are considering is for Thunderbird add-on authors to be able to easily benefit from existing jsm modules from the community such as protz's stdlib: https://github.com/protz/thunderbird-stdlib I agree, it would not take much to convert these to CommonJS, but that is less than ideal because: * this would be a fork, essentially * protz continues to maintain his code, so downstream maintenance would be a slight hassle * there are probably other cases like this, other jsm modules people will want to use easily from SDK code. Hopefully Matteo's point regarding loading jsms from the lib directory makes more sense to you given this use case.
> * this would be a fork, essentially > * protz continues to maintain his code, so downstream maintenance would be a slight hassle The way I've done it is to make a git clone, then modify the files. When upsteam changes, I just to "git pull" and have the changes. If you do it smartly, most merges will happen without conflicts and automatically. In fact, protz is a nice guy and I could imagine him supporting Jetpack, too. You did notice my point that even if you try to copy the JSM module verbatim into the Jetpack addon, it would not work, because JSMs normally reference chrome:// URLs to other JSM that they load, and to stringbundles (e.g. for error messages), so they often have to be adapted anyway, even with your proposal?
I took a look at https://github.com/protz/thunderbird-stdlib There is, for example: Cu.import("resource://gre/modules/FileUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); XPCOMUtils.importRelative(this, "../log.js"); let Log = setupLogging(logRoot+".SimpleStorage"); The "Cu.import("resource://gre/modules/FileUtils.jsm");" works, because "gre/modules/FileUtils.jsm" is part of Thunderbird and exists independent of the extension or Jetpack addon. XPCOMUtils.importRelative(this, "../log.js"); is smart. If you arrange your files as he expected, it works. However, if he did (and as most people do): Cu.import("chrome://myaddon/modules/log.js"); then it would fail, because "chrome://myaddon/" doesn't exist in Jetpack. Now, protz carefully designed this for easy drop-in to other extensions, so he used importRelative(), but I haven't seen that often. The same is true for string bundles, which I often need for error messages in my libs (I don't want to give the user of my lib the burder to add them all to their app code and locales).
In short, yes, protz' libs would work, but most others wouldn't work without changes anyway. For protz' libs and TB JSMs, wouldn't the requireJSM() suffice? Concretely, to your questions: > 1. You can "require" JS module as CommonJS module var FileUtils = requireJSM("resource://gre/modules/FileUtils.jsm"); > 2. The JS Module will stay in the /lib folder rather than /data folder – *it's* a library, it's not part of the content or assets, separation *is* important. Agreed. It could accept var FileUtils = requireJSM("protz/msgload.jsm"); as relative URL and load it from lib/, relative to the current module's location. > 3. The JS Modules don't messed up with any scope when required/imported The requireJSM() I gave above doesn't. var FileUtils = requireJSM("resource://gre/modules/FileUtils.jsm"); wouldn't change the current scope. You *can* load it in the current scope using requireJSM("resource://gre/modules/FileUtils.jsm", this); to get a behavior similar to Cu.import(), but it's optional. > 4. It's easily detected from the python tool as for `require("chrome")` > so it can > detect the type of access (load relative or absolute uri, for example). I can't talk about that, but I assume the tools can be extended to recognize requireJSM(). (But see end of comment 9.)
Ben, we're targeting JS Module available in the platform or general purpose library modules, mainly the Thunderbird stdlib Jeff mentioned (all the proposal was actually introduced by that). Any other JSM than is made for a specific add-on and rely on its path of course has to be changed. But is not what we're targeting. I think in that case we're talking about a porting of a "classic" add-on to a "SDK" add-on, so code change is already took in account. Of course, if the JS module made for the add-on is a general purpose library should works. For Thunderbird, they can just download from github the Thunderbird stdlib, put in your lib folder, and use it without change anything and just focused on add-on logic. As soon as the thunderbird stdlib will be ported to CommonJS modules, the add-on code won't change – just replace `requireJSM` with `require` or whatever we decided will be the name. That should helps for the transition to CommonJS modules. I don't think we want to all the scenarios, because we don't want to use JSM instead of CommonJS modules, it's just a feature that help you to port your existing code step by step, or reusing some general purpose library until is ported to CommonJS. About your reply on my comment, I was replying to Alex's comment. It's still not clear the syntax we want to use, the implementation is basically immediate but there are several things to take in account – mainly related to parsing from Python tool and security issues. For sure, we don't want provide something like: requireJSM("resource://gre/modules/FileUtils.jsm", this); Because that's is not very "CommonJS". The idea is makes JS Module and CommonJS module similar from the add-on code, so they could be switched later. Also, as Irakli said to you, we don't assure `this` will be point to the current scope in the future, so it's better don't rely on that for new feature.
(In reply to Ben Bucksch (:BenB) from comment #9) > > *without* needing to require chrome > > The last point is really important from our analysis and for add-on reviews. > > But as I said, this is true anyway. If a Jetpack addon uses JSM modules, > that's the same as having require("chrome") access. Sure, the difference is: it's not an arbitrary `require("chrome")` call. So exactly how we did for CommonJS module, if an add-on uses an internal JSM module, for instance (like "Services") we know the reviewer doesn't have to look at it. Or, if the add-on uses a JSM is commonly used and it's already reviewed (like Thunderbird stdlib) the reviewer can skip it. All this kind of information can be collected by the python parser and provided to the review easier if we have a way to recognize the use of a JSM instead of the manual use of `require("chrome")` and `Cu.import`. > And you can not just take any arbitrary JS module, convert it to a Jetpack > module, and then assume it's safe to expose. The API has to be written with > that in mind, and JSM APIs are not. They assume the caller has chrome rights > anyway and therefore they don't protect themselves against hostile callers. Definitely.
> exactly how we did for CommonJS module, if an add-on uses an internal JSM > module, for instance (like "Services") we know the reviewer doesn't have to > look at it. But that is not true! That's what I'm saying. JSM modules in general are *not* secure and you *do* have to review both the library and how the addon is using that lib, just like you have to review XPCOM calls whether they are used in a sane manner or not. If you want to make a secure boundary between chrome and sandboxes, the API has to be carefully designed for that from the start. JSM are not. Let's say Addon A uses Module M, and somebody reviews both, and doesn't see a problem, and let's them pass. Now Addon B uses Module M, but in a way that uses M to gain chrome rights. Reviewer looks at B, sees that it's only using M, sees that M was already reviewed, and lets B pass. > For sure, we don't want provide something like: > requireJSM("resource://gre/modules/FileUtils.jsm", this); > Because that's is not very "CommonJS". For sure, I do need that, because Cu.import() works like that and my existing code that uses this module is written with these symbols being in the main scope. *This* is actually something that may force me to change all my code, if I have to change every single assert() to my.assert(). Converting a JSM to a CommonJS module is trivial in comparison to this restriction that you want to impose here. (and I couldn't use var { foo } either, see ML.)
(In reply to Ben Bucksch (:BenB) from comment #17) > > exactly how we did for CommonJS module, if an add-on uses an internal JSM > > module, for instance (like "Services") we know the reviewer doesn't have to > > look at it. > But that is not true! That's what I'm saying. I think you missed part of what I said. I don't think you want to review a Services.jsm module that is already included in the platform, right? Also if, an add-on use a 3rd party modules, like Thunderbird stdlib that was already reviewed, the reviewer can skip that part – not how the module is used, but the module itself. Of course if we're talking about a JS module that is not part of the platform, or it was never reviewed before, it has to be *carefully* reviewed, because it has chrome access. > If you want to make a secure boundary between chrome and sandboxes, the API > has to be carefully designed for that from the start. JSM are not. > Let's say Addon A uses Module M, and somebody reviews both, and doesn't see > a problem, and let's them pass. Now Addon B uses Module M, but in a way that > uses M to gain chrome rights. Reviewer looks at B, sees that it's only using > M, sees that M was already reviewed, and lets B pass. As you said, the difference is how the module M is used from A and B, not from the Module itself. Of course, reviewing how B use the Module M could bring some security concern that makes the Module M reviewed again. But that's scenario it will apply also to CommonJS module that uses `chrome` access. Otherwise, I'm not sure I got your example. > > For sure, we don't want provide something like: > > requireJSM("resource://gre/modules/FileUtils.jsm", this); > > Because that's is not very "CommonJS". > For sure, I do need that, because Cu.import() works like that and my > existing code that uses this module is written with these symbols being in > the main scope. I remember you need that, but you can achieve that in several ways. You can always use `merge` as we discussed in the mailing list. Now we're targeting a different problem, as the subject of the bug said we want to use JS Module as CommonJS module, and the way you suggested is not CommonJS compliant.
I created a module on builder using Ben's code: https://builder.addons.mozilla.org/library/1040049/latest/ ...and an example: https://builder.addons.mozilla.org/addon/1040048/latest/ I'm going to blog about this later today.
> I don't think you want to review a > Services.jsm module that is already included in the platform, right? No, but you need to review every addon that uses Services.jsm in the same way that need to review an addon that uses require("chrome"). I'm assuming you want to somehow shortcut the review of the addon itself, because you said in comment 16: > it's not an arbitrary `require("chrome")` call. But Services.jsm allows to gain chrome, so you *do* have to treat it like an arbitrary "require('chrome')" call in your review. There's no difference for your review whether the dev does: var Services = {} require("chrome").Cu.import("resource://gre/modules/Services.jsm", services); or requireJSM("resource://gre/modules/Services.jsm") Both can do whatever mischief they want and you have to review the addon carefully. In both cases, you only need to review "resource://gre/modules/Services.jsm" once (or not at all, because it's part of Mozilla, and you know it's dangerous).
> I created a module on builder using Ben's code: > https://builder.addons.mozilla.org/library/1040049/latest/ > ...and an example: > https://builder.addons.mozilla.org/addon/1040048/latest/ > I'm going to blog about this later today. Thank you! Yes, I think (and hope) that this would satisfy most of your needs.
Jeff, I think is a bit premature. If you want to blog about how to use JS module from Jetpack it's perfect, so for instance how to write a "wrapper": // lib/addon-manager.js - AddonManager's wrapper require("chrome"); Cu.import("resource://gre/modules/AddonManager.jsm", exports); But if you want to blog about how to "require" them as CommonJS module it could be bring some confusion at this stage, especially because we don't defined yet how to do it. The code that Ben wrote it could be ok for his scenarios, but it has some downside in jetpack (the object returned is not an "exports", therefore some rules we applied on CommonJS modules are not used). If you're looking for feedback, I guess it could be more useful at that stage write a post about how to use JS Module in Add-on SDK, as Alex said, and mention that we're thinking to include in the Add-on SDK a simpler way to do it and collect the responses.
(In reply to Ben Bucksch (:BenB) from comment #20) > > I don't think you want to review a > > Services.jsm module that is already included in the platform, right? > No, but you need to review every addon that uses Services.jsm in the same > way that need to review an addon that uses require("chrome"). Sure. But you don't have to review the module itself, that what I'm saying. > I'm assuming you want to somehow shortcut the review of the addon itself, > because you said in comment 16: Yes, that's actually one of the reason to use Add-on SDK to write add-ons instead of the "classic" way. > > it's not an arbitrary `require("chrome")` call > But Services.jsm allows to gain chrome, so you *do* have to treat it like an > arbitrary "require('chrome')" call in your review. What I mean is: you don't have an "unknown" module that has chrome access to review all the time. You know the module is well-known module, so you don't have to review that specific module all the time. Of course you have to review how the module is used in the Add-on. > There's no difference for your review whether the dev does: > var Services = {} > require("chrome").Cu.import("resource://gre/modules/Services.jsm", services); > or > requireJSM("resource://gre/modules/Services.jsm") There is, actually. Because with the second option, we can collect this information and provide to the reviewer like we do for CommonJS module. The first one, it's just regular code.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #22) > Jeff, I think is a bit premature. If you want to blog about how to use JS > module from Jetpack it's perfect, so for instance how to write a "wrapper": > > // lib/addon-manager.js - AddonManager's wrapper > require("chrome"); > > Cu.import("resource://gre/modules/AddonManager.jsm", exports); I was only going to blog about that code in particular and how it could be used to solve problems like Ben describes. Simply: how do I use jsm modules in SDK code? Here is how! etc...
Do we really want to see JSM in jetpack? I believe we shouldn't. So by introducing something to use them as a common JS, but not really like a regular common JS module we end up complexifying things. So we may prefer to offer helpers to convert JSM to Common JS module instead of complexifying main SDK codebase in order to support them. I can understand your willingness to support them in order to ease porting old addons and have a nice security pattern around them. But is this really that important? Is this relevant to maintain code specific to JSM and spend time on this? We are currently discussing with Thunderbird team about how we could enable thunderbird support. protz is going to be involved in this discussion so he may work on his code and provide a real Jetpack variant of these modules.
> function requireJSM(url, targetScope) > { > var scope = targetScope ? targetScope : {}; > return Cu.import(url, scope); > return scope; > } Sorry, those 2 returns were of course nonsense. The first should be removed, i.e. correct is: function requireJSM(url, targetScope) { var scope = targetScope ? targetScope : {}; Cu.import(url, scope); return scope; } Sorry Jeff!
I agree with Alexandre. * If it's primarily only about protz's stblid, that should be trivial to offer as Jetpack module. * If it's about importing Thunderbird or Gecko JSMs, I think a requireJSM() utility function would help in the short term and Jetpack APIs as wrappers in the long term. * If it's about migrating old extensions with JSM to Jetpack, a tutorial with the tips above (e.g. comment 9) would help more. * * I did that migration to CommonJS modules for about 10 JSMs with 500-1000 lines each in my extension, and it went fairly smoothly, it wasn't a big deal. (What *is* a big deal is stuff like "you are not allowed to import into the global scope, that really is a killer in practice.)
So, after these discussions I agreed that probably we don't want introduce in the SDK any shortcut to load JSM. It's probably not worthy to introduce and support this feature, so I guess the best things we can do is: a) Port (or help protz to port) Thunderbird stblib as CommonJS b) Add to Add-on SDK documentation a page about how to port JSM module to CommonJS module, and provide a link to 3rd party modules that could help that process What do you think?
Sounds good to me.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #28) > So, after these discussions I agreed that probably we don't want introduce > in the SDK any shortcut to load JSM. It's probably not worthy to introduce > and support this feature, so I guess the best things we can do is: > > a) Port (or help protz to port) Thunderbird stblib as CommonJS > b) Add to Add-on SDK documentation a page about how to port JSM module to > CommonJS module, and provide a link to 3rd party modules that could help > that process Sounds like we have agreement here, Matteo, can you open a bug to document this, this bug has gotten quite longwinded so I think we'll just close it.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Whiteboard: [triage:followup]
You need to log in before you can comment on or make changes to this bug.