switch to build-time module resolution (from run-time lookup)

RESOLVED FIXED in 1.0b5

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: warner, Assigned: warner)

Tracking

unspecified
1.0b5
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Bug 591338 and bug 611495 both involve improvements to the way we do module-resolution in jetpack (the process that starts with require("foo") and ends with deciding which exact "foo.js" gets loaded). At present, we mainly do this resolution at runtime, in javascript, in securable-module.js (and others). But we also construct a manifest at buildtime (i.e. during "cfx xpi") and use it to restrict what the runtime loader is allowed to do.

I think it would be more reliable to consolidate this functionality into the SDK, and do only build-time resolution. The manifest would include a table that says "when module 4 does require('foo'), it should get module 5, which is loaded from FILE and has a hash of ABC123". Each require() function will then just do a simple table lookup, relying upon whatever $JSPATH-searching logic was done at build-time to generate that table.

One improvement of this change would be smaller XPIs: instead of including the full contents of any package that was around at build time, we'd only copy in the lib/ modules that are actually referenced. (we would need to copy in the full data/ directory of those packages, however, since the require("self").data methods are not constrained like require() itself).

One potential complication is with the test suite: previously we've allowed module-not-in-manifest to signal a warning, rather than an error, because we weren't sure exactly when test files ought to be included. We'd need to look more carefully at this, but I think it would be ok to skip the whole test/ subdirectory when creating an XPI.
Would modules that require("chrome") (or some other 'superuser' bit) still be allowed to dynamically require anything they please?
(Assignee)

Comment 2

8 years ago
(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

Updated

7 years ago
Blocks: 615060

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
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.
Attachment #525882 - Flags: feedback?(rFobic)
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.
(Assignee)

Comment 9

7 years ago
(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.
(Assignee)

Comment 10

7 years ago
Comment on attachment 525882 [details]
branch to review

switched to a different branch
Attachment #525882 - Flags: feedback?(rFobic)
(Assignee)

Comment 11

7 years ago
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.
Attachment #525882 - Attachment is obsolete: true
Attachment #527319 - Flags: review?(rFobic)
(Assignee)

Updated

7 years ago
Blocks: 620559
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+
(Assignee)

Comment 15

7 years ago
Landed, with a=myk, in https://github.com/mozilla/addon-sdk/commit/bf1d987e99c7fa8c056cf511cae0d7640af3f4c7
 . Yay!
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

7 years ago
oops, I missed a unixism (assuming os.path.sep=="/"), and tests are failing on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

7 years ago
> 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?
(Assignee)

Comment 18

7 years ago
(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 .
(Assignee)

Comment 19

7 years ago
buildbot is green once more, so I think I nailed the slash issue. Reclosing.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 20

7 years ago
> 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.