Would modules that require("chrome") (or some other 'superuser' bit) still be allowed to dynamically require anything they please?
(In reply to comment #1) > Would modules that require("chrome") (or some other 'superuser' bit) > still be allowed to dynamically require anything they please? Good question. We certainly want to allow that. The trick is in the bundling: if we don't know what they're going to ask for until run-time, how can we arrange to include the files in the XPI at build time? Atul and I were kicking around an idea on friday: we define a magic module named "everything". If, at build-time, we notice anything doing require("everything"), then we walk through every package on $JSPATH, figure out how you might want to import them (maybe both short require("panel") names and also the fully-qualified require("addon-kit/panel")), and copy all those modules into the XPI. Then a module could do something like: var require = require("everything").require; const dynamic = "panel"; var panel = require(dynamic); If you don't use require("everything"), you get a small XPI but then chrome-using modules' dynamically-constructed require() statements will be limited to the XPI contents. The use-case we've imagined for require("everything") would be a sort of playground system: an addon that behaves like the old Jetpack Prototype, with an in-browser text editor, on which you can type code one line at a time and run it right away without the SDK and without a "cfx run" step. Can you think of other use-cases for a dynamic require() statement? What would you use it for?
(In reply to comment #2) > Can you think of other use-cases for a dynamic require() statement? What > would you use it for? For me personally, I built an addon using the SDK, then did "cfx xpi" to create the XPI file. I then install that XPI into a Firefox profile and share it with the world. Because of where my main SDK dev environment is located (Linux virtual machine on my Windows host), I usually try hacking on to the installed extension's folder/(packed xpi file) in the profile to add new features to the extension. If those new features aren't covered by the modules already require()d by the extension back when "cfx xpi" was run, I have to require() the needed modules at run-time (or at least, when the browser restarts to pick up the changes). When I do this currently, I see the warning in the Error Console about how I shouldn't be doing this because in the future it won't work. But at the moment, this is the fastest way for me to play around with development. After I get a new feature (mostly) working in the installed extension, I then boot up the virtual machine and copy the new code over to the SDK and re-run "cfx xpi" to get a properly generated XPI file. Maybe not a common use-case, but one that's valid for me.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Summary: switch to build-time module resolution? (from run-time lookup) → switch to build-time module resolution (from run-time lookup)
Target Milestone: --- → 1.0
This capability sounds similar to how the RequireJS optimizer works: http://requirejs.org/docs/optimization.html It will bundle all the modules needed for for a project into one JS file. It loads each file, runs it through require.js, (but only a simplified, AST-parsed form that just has the dependencies) so it traces the dependency tree that way. The build profile has a way to specify additional modules that should be included that cannot be statically determined (so for example for dynamic require cases). The optimizer runs as pure JS, with adapters to run inside Node or Rhino. It is probably possible to provide a Spidermonkey adapter too. Since all the modules are bundled into one file (wrapped in AMD wrappers), that is probably different than what is desired for this bug, where the jetpack loader uses a separate context for each module, but it may also have a benefit of just having one file to load/checksum. And with that, a simplified loader could be used, which does not do other file loads. That said, it may be undesirable to have a different execution context and loader code for the built/vs non-built case for testing/consistency purposes. It is probably not a direct fit for this use case, but just mentioning some tools or code that may be useful to help solve the issue.
incidentally, https://github.com/warner/addon-sdk/tree/simpler-linker3 is where I'm currently working on this. As of this morning, I've got all the unit tests passing again except for the recent async/define ones: I'm still looking over that code to see what needs updating.
Created attachment 525882 [details] branch to review Ok, this branch is finally passing all tests. I plan to clean it up a bit before asking for review for real, but Irakli it'd be great if you could start taking a look at it and give me some feedback.
Brian do you mind creating a pull request for that branch that way it will be easier to see the changes and put comments. Also your further cleanup changes will be picked up by it as long as you don't rebase.
So one thing that I noticed is: addon-sdk)➜ repl (master) cfx run Warning: unable to satisfy require(./events) from ModuleInfo [moz-js lib net] (/Users/gozala/Projects/addon-sdk/packages/moz-js/lib/net.js, None) Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'. Using profile at '/var/folders/-y/-yjxO9IXGGSRN-hSaip1Qk+++TM/-Tmp-/tmpdK8eGW.mozrunner'. TypeError: this._pathMap[path] is undefined (resource://jep4repl-api-utils-lib/securable-module.js:554) stack: getFile("resource://jep4repl-api-utils-lib/unload.js")@resource://jep4repl-api-utils-lib/securable-module.js:554 loadFromModuleData([object Proxy])@resource://jep4repl-api-utils-lib/securable-module.js:293 loadMaybeMagicModule("unload",[object Proxy])@resource://jep4repl-api-utils-lib/securable-module.js:280 syncRequire("unload")@resource://jep4repl-api-utils-lib/securable-module.js:229 asyncRequire("unload")@resource://jep4repl-api-utils-lib/securable-module.js:345 @:0 @resource://jep4repl-api-utils-lib/securable-module.js -> resource://jep4repl-api-utils-lib/memory.js:137 FAIL Total time: 1.849899 seconds Program terminated unsuccessfully. What is interesting there is that package I'm trying to run does not depends on "moz-js" package which I have in my packages folder, it's just happens so that moz-js and my `net` package both have `net` modules.
(I want to rewrite the patch before adding the pull-request, and rebase it as well, but I'll try to do that tomorrow, and then stop rebasing it) Weird. If you build the XPI and look in harness-options.json at the manifest, can you see how moz-js/net.js is being included? Do you think that the dependency-search code is finding the wrong module? Maybe a module is using require("net") intending to grab the "net" package's main.js, but the linker is instead satisfying the dependency with moz-js/net.js (from the wrong package altogether)? I don't think we have any support for the "main" property in package.json yet.
Comment on attachment 525882 [details] branch to review switched to a different branch
Created attachment 527319 [details] pointer to pull request 148 here's the new rebased/rewritten branch. Tests finally pass, and some debug noise has been cleaned up. Ready for review.
Hmm, I think we're targeting this to 1.0b5. That's what the 1.0b5 plan claims, and it's a big enough change that we should get beta coverage for it.
Target Milestone: 1.0 → 1.0b5
Brian, I managed to get my head around it and peer review we talked about is no longer necessary. Overall I'm happy with a change but I have a few nits and open questions (see pull request). I'd like to hear from you on before accepting this, I'll be online.
Comment on attachment 527319 [details] pointer to pull request 148 After discussing few questions I had on IRC, I'm happy with a change. Thanks brian!!
Attachment #527319 - Flags: review?(rFobic) → review+
Landed, with a=myk, in https://github.com/mozilla/addon-sdk/commit/bf1d987e99c7fa8c056cf511cae0d7640af3f4c7 . Yay!
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
oops, I missed a unixism (assuming os.path.sep=="/"), and tests are failing on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> One improvement of this change would be smaller XPIs Was this actually implemented? (I'm getting a large XPI when building and not sure if it's just me.) If not, do you plan to work on it and is there a separate bug on that?
(In reply to comment #17) > > One improvement of this change would be smaller XPIs > Was this actually implemented? (I'm getting a large XPI when building and not > sure if it's just me.) Alas, I had to defer that feature to get the patch small enough to review in time for 1.0b5 (I'd actually forgotten that I made that decision a month ago). I've opened Bug 653256 to track that one, and I'll try to get it in place for 1.0 .
buildbot is green once more, so I think I nailed the slash issue. Reclosing.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago → 7 years ago
Resolution: --- → FIXED
> Alas, I had to defer that feature Thanks for the clarification!
You need to log in before you can comment on or make changes to this bug.