596932 - Support CommonJS Asynchronous Module Definition proposal define (previously require.def) idiom (like used by RequireJS)

Status: RESOLVED FIXED

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

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 updated version of jrburke's patch

In Mozilla Messaging land we are doing a lot of CommonJS work that wants to run on the web and also in Jetpack.  There is a consensus currently being reached on the CommonJS list about supporting a wrapping syntax along the lines of that used by RequireJS, so this would not be totally crazy.

It would be great if Jetpack could support this syntax 'out of box'.

http://wiki.commonjs.org/wiki/Modules/AsynchronousDefinition

As a discussion starter, I am attaching an updated version of James Burke's earlier patch to this end as documented on his blog post:
http://tagneto.blogspot.com/2010/05/using-requirejs-syntax-in-jetpack.html

Besides fixing bit rot, the patch:

- Avoids removing the findSandboxForModule function used for testing access to a sandbox's global scope.  However, it still will only usefully work for non-require.def module (unless you involve a debugger...)

- Adds regex detection of the require.def("module name", ["dep1", "dep2", ...], function () {...code...}) idiom for manifest.py, the thing that builds the manifest that stops the cuddlefish loader from getting deeply upset about everything.

I am assigning this to me to convey that we (myself and James) are willing to work this to completion, but if someone on the jetpack team wants to take it on instead, we will not complain.  I recognize you are currently in 0.8 release lockdown and so may not be able to respond to this immediately.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 make the moduleOverrides mechanism work too...

Comment 2

[Atul Varma [:atul]]

Thanks Andrew and James! That's cool, we should add this to Jetpack. It'll also help with some Firebug Lite integration that Kevin Dangoor would like to get done so that we can move Mozilla's/Firefox's developer tools to work as Jetpack-based addons.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Atul: f+?

Andrew: ready for review?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

The patch is still what I am using locally, so it is up-to-date.

I would expect a 'final' patch to:
- Have at least one unit test.
- Support the 'toString' automated detection of dependencies that has been provisionally adopted for the asynchronous module definition idiom.
- Not have the comments in it that say how it is a patch.

I was expecting some implementation direction before we got to that stage.  If Atul is generally fine with the implementation strategy and does not feel it would be best for him to modify the existing code, I can proceed straight to tricking James into doing the above by claiming the tiki people are going to rewrite the patch if he does not ;).

Comment 5

[Atul Varma [:atul]]

Comment on attachment 475808 [details] [diff] [review]
v2 make the moduleOverrides mechanism work too...

Looks good... Aside from unit tests, it'd be nice to also have this documented for end-users, perhaps by changing the description of require() in the "Jetpack Globals" section of the documentation.

Also, stylistically it'd be cool if the indents were 2 spaces instead of 4, and if there was a space between '//' and the beginning of the comment, e.g. '//lorem ipsum' -> '// lorem ipsum'.

Right now warner is working on bug 591525, which may touch some of this code, and I'm working on adding e10s support to Jetpack [1], which definitely touches some of the same code, so I'm not sure how we should orchestrate the landings. At the very least, though, a hearty set of unit tests should help ensure that we don't regress regardless. :)

[1] http://github.com/toolness/jetpack-e10s/

Comment 6

[James Burke]

Attachment: V3 of the patch that supports anonymous, async modules with test cases

Here is an updated patch with style fixes and tests. securable-module.js seems to use a 3-space starting indent. I can generate a separate patch that converts it to start with two spaces, but for this patch I kept all that the same so that only the new code can be reviewed easier. The new code should all use the two-space indent, lined up to the nearest 2 space aligned starting point.

All tests pass and do not introduce any new failures when run as part of cfx testall.

This version of the patch adds support for anonymous modules and the shortened, function-only require.def that uses Function.prototype.toString() to discover the require dependencies:

require.def(function(require, exports, module) {});

So this patch should round out the async module support as specified here:

http://wiki.commonjs.org/wiki/Modules/AsynchronousDefinition

I generated the patch use git, and was able to apply it to the hg repo via a patch -p1 command. The changeset in GitHub can be seen here:

http://github.com/jrburke/jetpack-sdk/commit/866a89865867349acac0d924850e77297d0b976e

If you prefer a github pull request, I can do that too.

I will attach a separate patch with a small update to the Jetpack Globals doc that mentions require.def and the async module proposal.

Comment 7

[James Burke]

Attachment: Doc patch for Jetpack Globals

This is the doc patch that mentions require.def and the async module proposal. It is also on GitHub at this location:

http://github.com/jrburke/jetpack-sdk/commit/0e92b70dd5520e445de39d77e72f9c628bd2d2d3

I can do other more extensive docs if someone wants to outline the context for them and where best to place them.

Comment 8

[Atul Varma [:atul]]

Comment on attachment 482006 [details] [diff] [review]
V3 of the patch that supports anonymous, async modules with test cases

Sorry I didn't get to this earlier. The problem now is that this patch has rotted against the onslaught of bug 591525, and is likely to rot even more as warner and I are pair-programming on bug 567703 on this branch:

  http://github.com/toolness/jetpack-sdk/tree/e10s

Today warner and I are going to add logic to manifest.py to comply with the e10s loading scheme, which is documented here:

  http://github.com/toolness/jetpack-e10s/#readme

Do you mind waiting until we modify manifest.py, and then you can merge our changes into your branch, fix what's needed, and issue a pull request to our branch?

Comment 9

[Atul Varma [:atul]]

Comment on attachment 482008 [details] [diff] [review]
Doc patch for Jetpack Globals

See my above comment.

Comment 10

[James Burke]

(In reply to comment #8)
> Do you mind waiting until we modify manifest.py, and then you can merge our
> changes into your branch, fix what's needed, and issue a pull request to our
> branch?

Sure thing. I will watch bug 567703 to see when the manifest.py changes go in, and I can redo the patch. I need to redo it anyway given some changes prompted by the CommonJS list. It is probably good to have a bit more time anyway to see if there is anything else that comes out of the discussions on the CommonJS list.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Atul, given that the fix for bug 567703 has landed on jetpack trunk, is it cool to resume work on this bug or are there still more changes planned for the loader and manifest logic?

Comment 12

[Atul Varma [:atul]]

Yeah! Bust it up, there aren't any urgent changes we need to make to the loader/manifest logic. While we'd like to attend to bug 591338 sometime soon-ish, nobody's working on it right this minute, and given that you guys are really quick at implementing require.def(), I'd rather just land yours first before anyone works on 591338.

Also, if you could continue to do your stuff on github that would be easiest, since that way we can just merge-in changesets and whatnot. patches blow.

That said, please do upload some sort of "placeholder" attachment--it can be the actual patch if you want, but it doesn't have to be--and ask for review from me on it once you've got a pull request ready. that way it'll show up on my bugzilla dashboard.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 14

[James Burke]

Attachment: Placeholder file to ask for review of a Git pull request.

Adding placeholder file per Atul's instructions to signal a review request for the latest patch visible at this pull request:

https://github.com/mozilla/addon-sdk/pull/37

Comment 15

[Atul Varma [:atul]]

Thanks!

I've merged this into the latest master on a branch of my repository here:

  https://github.com/toolness/jetpack-sdk/tree/jrburke-master

You'll see from the merge that there were some minor documentation conflicts with a few recent changes to globals.md, so I've fixed those. Will review the code shortly.

Comment 16

[Atul Varma [:atul]]

Comment on attachment 491453 [details] [review]
Placeholder file to ask for review of a Git pull request.

Overall, I think this looks great, but the one blocker is that I'd really like it if we could have unit tests for the new idiom-detectors added to `manifest.py`. Would you mind adding those?

Comment 17

[James Burke]

(In reply to comment #16)
> Overall, I think this looks great, but the one blocker is that I'd really like
> it if we could have unit tests for the new idiom-detectors added to
> `manifest.py`. Would you mind adding those?

Sure thing. Based on the feedback in the commit, I will do the following:

* Add tests for manifest.py
* use Array.isArray since the addon-sdk is targeting Firefox 4 and greater, and consequently removing the isArray function
* Use typeof (it) === "function", and remove the isFunction function.
* rename require to syncRequire inside _makeApi

Comment 18

[Atul Varma [:atul]]

Awesome, that sounds great, thanks!

Comment 19

[Atul Varma [:atul]]

Oh, and don't worry about the e10s-ification, I can work on that, as it will help me familiarize myself better with your code.

Comment 20

[James Burke]

Attachment: Placeholder file to ask for review of a Git pull request.

A new pull request: https://github.com/mozilla/addon-sdk/pull/69

that includes feedback that was given on a previous pull request (37):

    * Tests for manifest.py changes, and subsequent improvements in the regexp parsing for callback-based require/define calls.
    * isFunction replaced by typeof it === 'function' tests
    * isArray replaced with Array.isArray
    * Private require function in _makeApi is named syncRequire

Comment 21

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am not sure whether I've got the latest version of the securable-module.js with async loading support, but the require function (line 475) should also use a second "callback" parameter and pass it along so, it's propagated into asyncRequire method (if applied).

require: function require(module, callback) {
    return (this._makeApi(null).require)(module, callback);
},

Consequently, the callback is actually executed:

var loader = new SecurableModule.Loader(...);
loader.require(["subtract"], function(module)
{
    var subtract = module.subtract;
    sysout("3 - 1 = " + subtract(3, 1));
});

Does that make sense?

I have cloned:
https://github.com/jrburke/addon-sdk.git

Honza

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yet couple of questions.

1) When using the securable-module in chrome space (according to https://wiki.mozilla.org/Labs/Jetpack/JEP/25) I am seeing a problem with the Array.isArray (Array is undefined).

Error: Array.isArray is not a function (Line: 322)

2) I have been forced to use following syntax:

define(["require", "exports", "module"], function(require, exports, module)
{
   // ...
}

Instead of:

define(function(require, exports, module)
{
   // ...
}

Is that correct?

Honza

Comment 23

[Atul Varma [:atul]]

Thanks James!

So I spoke with warner yesterday about this patch and after perusing the code, we have a few security-related questions. It might be easier to just have a phone conference or skype call about this and then document the requirements and create test cases for them.

For example, what happens if a file has multiple define() calls? I assume that this was originally created for cases where a Web page could include a <script> tag that defines multiple modules in the same file, but there's not really an analog of the <script> tag in a Jetpack sandbox...

Also, right now our security model assumes that each file executes in its own sandbox and its global scope can constitute a unit of isolation. If multiple modules are defined in the same global scope, and assuming we don't actually subset the language using e.g. ADsafe, those modules aren't provably isolated from one another, which isn't something our aspirational security model currently has a concept of. (Though in reality, no modules are yet isolated from one another, which probably makes things a bit confusing for you.)

We also want to make sure that it's not possible for e.g. a define() call to define a module called "foo", and then for a "foo.js" file to also define a module called foo, and have the manifest checker claim that one of them is the "real" foo that will be loaded at runtime while the runtime actually believes the other will be the "real" foo.

I suspect that a lot of this is confusing to you, and it is to me as well, in part because we're trying to enforce a level of isolation between modules that the rest of CommonJS doesn't really assume.

Do you have time to talk sometime late next week, on either Thursday or Friday? Warner is out of town on PTO at the beginning of the week, which is why we can't meet then.

Comment 24

[Atul Varma [:atul]]

Comment on attachment 494461 [details] [review]
Placeholder file to ask for review of a Git pull request.

I'm going to r- this for now so it gets off my review radar, and we'll loop back on it once we've talked through the security concerns with warner and jrburke.

Comment 25

[James Burke]

(In reply to comment #22)
> Yet couple of questions.
> 
> 1) When using the securable-module in chrome space (according to
> https://wiki.mozilla.org/Labs/Jetpack/JEP/25) I am seeing a problem with the
> Array.isArray (Array is undefined).
> 
> Error: Array.isArray is not a function (Line: 322)

What version of Firefox are you using? Array.isArray is in Firefox 4b7 and greater, I believe.

> 2) I have been forced to use following syntax:
> 
> define(["require", "exports", "module"], function(require, exports, module)
> {
>    // ...
> }
> 
> Instead of:
> 
> define(function(require, exports, module)
> {
>    // ...
> }
> 
> Is that correct?

I will confirm that define(function(require, exports, module){}) should work. A form of it is in the unit tests, in red.js and tiger.js Maybe it is a result of issue #1? I will add a specific test for this case too, and perhaps try the type of embedding you are using.

I will also look at the require callback issue specified in Comment 21, that sounds straight-forward.

Comment 26

[James Burke]

(In reply to comment #23)
> Do you have time to talk sometime late next week, on either Thursday or Friday?
> Warner is out of town on PTO at the beginning of the week, which is why we
> can't meet then.

I will contact both of you out of band to set up a meeting time. The concerns are reasonable, and I'll look at addressing them in the patch in the meantime.

Comment 27

[John J. Barton]

Firebug project is using a version of this code. In trying to debug code loaded with securable-module.js we ran in to several problems.

1) The first use of a sandbox is: 
  sandbox.evaluate("var exports = {};");
If it is 
  sandbox.evaluate({contents: "var exports = {};", filename: module_info.filename});
then the sandbox has a useful filename when it is created.

2) These lines:
if (this._principal == systemPrincipal)
   options.filename = maybeParentifyFilename(options.filename);
write over the filename with a string that is not a filename. Omitting these lines makes the sandbox evaluation mozilla data structure (jsdIScript) have a legitimate file name.

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to comment #25)
> (In reply to comment #22)
> > Yet couple of questions.
> > 
> > 1) When using the securable-module in chrome space (according to
> > https://wiki.mozilla.org/Labs/Jetpack/JEP/25) I am seeing a problem with the
> > Array.isArray (Array is undefined).
> > 
> > Error: Array.isArray is not a function (Line: 322)
> 
> What version of Firefox are you using? Array.isArray is in Firefox 4b7 and
> greater, I believe.
Ah, sure, I was testing with Firefox 3.6 (now I noticed the comment 20 where you mentined the switch to Array.isArray.

> > 2) I have been forced to use following syntax:
> > 
> > define(["require", "exports", "module"], function(require, exports, module)
> > {
> >    // ...
> > }
> > 
> > Instead of:
> > 
> > define(function(require, exports, module)
> > {
> >    // ...
> > }
> > 
> > Is that correct?
> 
> I will confirm that define(function(require, exports, module){}) should work. A
> form of it is in the unit tests, in red.js and tiger.js Maybe it is a result of
> issue #1?
Yes, this was related to the isArray problem.

> I will add a specific test for this case too, and perhaps try the
> type of embedding you are using.
> 
> I will also look at the require callback issue specified in Comment 21, that
> sounds straight-forward.

Thanks!
Honza

Comment 29

[James Burke]

Attachment: Placeholder file to ask for review of a Git pull request.

I have updated pull request 69:
https://github.com/mozilla/addon-sdk/pull/69

with two commits:

1) A commit to prevent multiple define calls in a file and only allow named define() modules that match the expected name. With these two constraints, I believe it satisfies the other concern about being able to redefine a module later. The checks are done in securable-module.js. If you want something done in manifest.py, please let me know, but I was under the impression that file mostly confirms expected dependencies:
https://github.com/jrburke/addon-sdk/commit/a2410b84474b66331a350a17f323f13aea8fda04

2) Pick up Honza's fix for calling the callback-style require with a new loader instance:
https://github.com/jrburke/addon-sdk/commit/d72e21ab92415598225053180048858b7e40f444

A problem with commit #2: I added a test to the test-securable-module.js file to confirm the change, but I do not believe that test file gets run, or if it does, the output is somehow ignored. I tried just putting a throw near my test, and it did not cause a failure when running the tests in that package or as part of testall. If I did something incorrect, please clue me in.

I understand that you may be busy and discussion of the patch may wait until next week.

Comment 30

[James Burke]

(In reply to comment #27)
> Firebug project is using a version of this code. In trying to debug code loaded
> with securable-module.js we ran in to several problems.
> 
> 1) The first use of a sandbox is: 
>   sandbox.evaluate("var exports = {};");
> If it is 
>   sandbox.evaluate({contents: "var exports = {};", filename:
> module_info.filename});
> then the sandbox has a useful filename when it is created.
> 
> 2) These lines:
> if (this._principal == systemPrincipal)
>    options.filename = maybeParentifyFilename(options.filename);
> write over the filename with a string that is not a filename. Omitting these
> lines makes the sandbox evaluation mozilla data structure (jsdIScript) have a
> legitimate file name.

I believe these two issues are unrelated to the patch for this ticket. I have not touched the lines in question, so I believe them to be issues that predate this patch.

I can look at applying a fix as part of this patch, but I feel it would be best to at least have a separate ticket to track the fix.

Comment 31

[John J. Barton]

(In reply to comment #30)
...
> I believe these two issues are unrelated to the patch for this ticket. I have
> not touched the lines in question, so I believe them to be issues that predate
> this patch.
> 
> I can look at applying a fix as part of this patch, but I feel it would be best
> to at least have a separate ticket to track the fix.

Is there any other venue where any one of the readers here would care to discuss this code? I'm re-writing it for firebug use and to support debugging of extensions. I have questions about the loader API.

Comment 32

[James Burke]

(In reply to comment #31)
> Is there any other venue where any one of the readers here would care to
> discuss this code? I'm re-writing it for firebug use and to support debugging
> of extensions. I have questions about the loader API.

For general addon SDK loader issues I would probably ask here:
http://groups.google.com/group/mozilla-labs-jetpack

If it is for something related to the define() API used in this fork:
https://github.com/jrburke/addon-sdk

Then I suggest contacting me directly. I just updated my fork to have the latest code from mozilla/addon-sdk.

For the two particular issues you mention, it looks like the latest code at:
https://github.com/mozilla/addon-sdk

has changed, and may have addressed the original issues you mentioned in this ticket (or just moved them around).

Comment 33

[Brian Warner [:warner :bwarner]]

Atul and I are looking at this. A quick merge from
https://github.com/mozilla/addon-sdk/pull/69 to current trunk shows at least
one test failure:

error: TEST FAILED: test-securable-module.testSecurableModule (exception)
error: An exception occurred.
Traceback (most recent call last):
  File "resource://testpkgs-api-utils-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://testpkgs-api-utils-lib/unit-test.js", line 223, in 
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://testpkgs-api-utils-lib/unit-test.js", line 248, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://testpkgs-api-utils-lib/unit-test.js", line 266, in start
    this.test.testFunction(this);
  File "resource://testpkgs-api-utils-lib/unit-test-finder.js", line 57, in runTest
    test(runner);
  File "resource://testpkgs-api-utils-tests/test-securable-module.js", line 60, in 
    file);
  File "resource://testpkgs-api-utils-tests/test-securable-module.js", line 250, in run
    loader.require(["subtract"], function (mod) {
  File "resource://testpkgs-api-utils-lib/securable-module.js", line 483, in require
    return (this._makeApi(null).require)(module, callback);
  File "resource://testpkgs-api-utils-lib/securable-module.js", line 287, in asyncRequire
    asyncMain(null, basePath, null, deps, callback);
  File "resource://testpkgs-api-utils-lib/securable-module.js", line 415, in asyncMain
    deps.forEach(function (dep) {
  File "resource://testpkgs-api-utils-lib/securable-module.js", line 437, in 
    syncRequire(dep);
  File "resource://testpkgs-api-utils-lib/securable-module.js", line 223, in syncRequire
    throw new Error('Module "' + module + '" not found');
Error: Module "subtract" not found

Also, a tiny style nit, some of the resulting diff shows lines with changes
to whitespace only.. it'd be nice to remove those from the diff.

I think we've talked about the overall feature well enough and we're happy
with it, so I think all that's left is to get the patch into shape. I'll try
to see if I can figure out the test failure. Atul has agreed to review it
once that's working.

Comment 34

[Brian Warner [:warner :bwarner]]

Oh, also, is https://github.com/mozilla/addon-sdk/pull/69 a replacement for https://github.com/mozilla/addon-sdk/pull/37 ? Should we close the older pull request?

Comment 35

[James Burke]

(In reply to comment #34)
> Oh, also, is https://github.com/mozilla/addon-sdk/pull/69 a replacement for
> https://github.com/mozilla/addon-sdk/pull/37 ? Should we close the older pull
> request?

Yes, 69 replaces 37, please close it out. I'll look at the test failure tonight, along with the whitespace only changes. I'll also update the patch/pull to be against the most latest master, my fork is a bit behind.

Comment 36

[James Burke]

(In reply to comment #33)
> Atul and I are looking at this. A quick merge from
> https://github.com/mozilla/addon-sdk/pull/69 to current trunk shows at least
> one test failure:

The test failure is fixed: that was a test I added but at the time the test framework did not seem to run that file when I added it. It now runs so I was able to fix the test since I was just guessing before. I also removed the whitespace changes from cuddlefish.js.

https://github.com/mozilla/addon-sdk/pull/69 now shows these updates. Please let me know if you think the patch needs other updates.

All the tests in api-utils pass, but when I tried to do a testall from the addon-sdk folder, I got the following error which also happens in mozilla/addon-sdk at the moment, so I cannot confirm the rest of the tests working. However before, all the other tests passed (or at least the ones passed that passed without this change).

Current testall error:

Testing cfx...
Traceback (most recent call last):
  File "/Users/jr/git/addon-sdk/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "/Users/jr/git/addon-sdk/python-lib/cuddlefish/__init__.py", line 427, in run
    test_all(env_root, defaults=options.__dict__)
  File "/Users/jr/git/addon-sdk/python-lib/cuddlefish/__init__.py", line 264, in test_all
    result = test_cfx(env_root, defaults['verbose'])
  File "/Users/jr/git/addon-sdk/python-lib/cuddlefish/__init__.py", line 289, in test_cfx
    retval = cuddlefish.tests.run(verbose)
  File "/Users/jr/git/addon-sdk/python-lib/cuddlefish/tests/__init__.py", line 55, in run
    tests = get_tests()
  File "/Users/jr/git/addon-sdk/python-lib/cuddlefish/tests/__init__.py", line 44, in get_tests
    optionflags=doctest_opts
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 2362, in DocFileTest
    test = parser.get_doctest(doc, globs, name, path, 0)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 609, in get_doctest
    return DocTest(self.get_examples(string, name), globs,
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 623, in get_examples
    return [x for x in self.parse(string, name)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 585, in parse
    self._parse_example(m, name, lineno)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 655, in _parse_example
    lineno + len(source_lines))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/doctest.py", line 741, in _check_prefix
    (lineno+i+1, name, line))
ValueError: line 47 of the docstring for xpi.md has inconsistent leading whitespace: '</pre>'
(

Comment 37

[Brian Warner [:warner :bwarner]]

that </pre> doctest failure has a pending patch: check out the patch on bug 625938. I'll take a look at the pull request combined with the doctest patch and try it out.

Comment 38

[Atul Varma [:atul]]

Comment on attachment 495761 [details] [review]
Placeholder file to ask for review of a Git pull request.

This looks good, with one nit, which is around here in securable-module.js:

  https://github.com/jrburke/addon-sdk/commit/c312dbec6ac1d4d4c300b0c6ff0f25000b962786#commitcomment-206286

Basically I am wondering if the regexp-based scanning done by manifest.py is enough to deal with this RequireJS use case. If we can't get rid of this regexp-scanning-at-runtime, then a quick explanation of why we're doing regexp-scanning at both build time and runtime in a comment above the runtime scanning code would be really helpful.

Comment 39

[James Burke]

(In reply to comment #38)
> Basically I am wondering if the regexp-based scanning done by manifest.py is
> enough to deal with this RequireJS use case. If we can't get rid of this
> regexp-scanning-at-runtime, then a quick explanation of why we're doing
> regexp-scanning at both build time and runtime in a comment above the runtime
> scanning code would be really helpful.

I just pushed a commit of a comment that explains why the regexps are done at runtime:
https://github.com/jrburke/addon-sdk/commit/34ebb4e3322283d1d16169f440898bf3ae99e746

Comment 40

[Brian Warner [:warner :bwarner]]

Based on Atul's r+, I've landed this in https://github.com/mozilla/addon-sdk/commit/27641da79cfaae436ce89f6641c05fb78d1b4f4f

Reading the comment you added in
https://github.com/mozilla/addon-sdk/commit/34ebb4e3322283d1d16169f440898bf3ae99e746
, I think I might see what you mean about the runtime scanner. It's a very
subtle point, and I'm pretty sure I've misunderstood it at least twice today
already. Here's my attempt to explain it.. please tell me if I'm on the right
track.

The link-time-generated manifest tells us which modules might be loaded by
all the define() calls in a single file, but any given define() may or may
not actually use any given one, and will access them at various times, say in
something like this:

=== main.js
// marker(1);
define( function(require,exports,module) {
             // marker(2);
             var a = require('A');
        } );
// marker(3);
define( function(require,exports,module) {
             var b = require('B');
        } );
===

In an async context, the body cannot be evaluated until all the require()
calls are prepared to return synchronously. If we rely upon the link-time
manifest, that means we'd need to pre-emptively load both A and B before
evaluating any of the function bodies, which means both A and B will have
been loaded by the time we get to marker(1):

 at marker(1): A loaded, B loaded
 at marker(2): A loaded, B loaded
 at marker(3): A loaded, B loaded

If, instead, we allow define() to scan its function body at runtime, then we
could defer the loading of B until later:

 at marker(1): A not loaded, B not loaded
 at marker(2): A loaded, B not loaded
 at marker(3): A not loaded, B not loaded

If A and B have side-effects, someone might be able to observe this
difference in execution order. Imagine that all three files,
main.js+A.js+B.js, have access to the same marker() function, which is
recording the order in which it is called, and that A.js calls marker("A"),
and B.js calls marker("B"). Then you might see:

 A,B,1,3,2  (using link-time manifest)

or

 1,A,2,3,B  (using run-time scanner)

or even

 1,3,A,2,B  (super lazy run-time scanner)

Is this difference in execution order the reason for doing the runtime scan?

Does this seem right? Does the define() spec require a specific execution
order? It seems to me that A+B might have been loaded much earlier anyways,
like if some unrelated code had referenced them too, so I'm not sure we could
guarantee that execution order even with the scanner.

My overall goal is to simplify the loader, and if we can remove the runtime
scanner, I think that might be easier to maintain in the long run. Since
Jetpack will load modules synchronously, at least from the point of view of
the code doing require() or define(), I'm not yet convinced that we need to
implement the scanner. But maybe I'm missing something?

I'll leave this ticket open a bit longer to solicit comments. But the code has landed.

Comment 41

[John J. Barton]

The simple way to avoid the scanner is use AMD syntax to define modules:

define("a.js",["b.js"], function(b) { return a; }); 
define("b.js",["a.js"], function(a) { return b; });

I think this is a better design in any case: scanning is a hack and we surely have enough of those already. If we are determine to use includes then putting them all at the top is better anyway.

Comment 42

[Brian Warner [:warner :bwarner]]

(In reply to comment #41)
> The simple way to avoid the scanner is use AMD syntax to define modules:
> 
> define("a.js",["b.js"], function(b) { return a; }); 
> define("b.js",["a.js"], function(a) { return b; });

Yeah. I *think* that with our existing manifest, we have enough information
to deduce the AMD information without that, although it would be pretty
coarse. manifest.py builds a mapping from filename to require() list, which
would be inefficient if you had multiple define() statements in a single
file, whereas the JS scanner can focus on a single define() body. OTOH, I
don't think we allow multiple define() statements in a single jetpack module,
as that sort of goes against the security model we're working towards.

Plus, relying upon that manifest would make our implementation very different
from how define() is done in things like node.js, which are all going to use
the JS scanner because they don't have an out-of-band manifest.py pass. But,
I'm not sure that difference in implementation is actually visible to the
application.

OT3H, I don't think we even need to figure out the dependency list ahead of
time: unlike the web-page context, Jetpack can just block in the require()
calls. OT4H, it looks like the AMD syntax you show above doesn't even have
any require() calls, and just expects the dependencies to be passed in as
arguments to the callback, which blows that idea out of the water.

> I think this is a better design in any case: scanning is a hack and we
> surely have enough of those already. If we are determine to use includes
> then putting them all at the top is better anyway.

I'm +0 on removing the scanner, since less code is always better. I've opened
a new ticket for discussing that option (bug 627073) and will close this one
out.

Comment 43

[John J. Barton]

Sorry I was not clear. I meant for each define() to be in its own file.

Comment 44

[Brett Zamir]

Could I request here that this be documented within the SDK docs, e.g., at https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/guides/commonjs.html ?