Last Comment Bug 872421 - [Chrome Workers] Provide a module loader for chrome workers
: [Chrome Workers] Provide a module loader for chrome workers
Status: RESOLVED FIXED
[Async:P1]
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla24
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on: 583083 679189 832609 873020
Blocks: WorkForTheWorkers 880664 881652 880663 883614 905109
  Show dependency treegraph
 
Reported: 2013-05-15 00:55 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2013-08-14 04:21 PDT (History)
14 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First prototype (11.23 KB, patch)
2013-05-15 06:53 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Module loader for workers (10.79 KB, patch)
2013-05-17 14:03 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
nfroyd: feedback+
Details | Diff | Splinter Review
Module loader for workers, v2 (23.00 KB, patch)
2013-05-22 08:15 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
nfroyd: feedback+
mh+mozilla: feedback-
Details | Diff | Splinter Review
Module loader for workers, v3 (23.18 KB, patch)
2013-05-23 11:38 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
rFobic: review+
Details | Diff | Splinter Review
Module loader for workers, v4 (24.67 KB, patch)
2013-06-10 05:30 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 00:55:10 PDT
For inclusion purposes, Workers provide |importScripts()|, but this function behaves as a synchronous implementation of <script src = "...">, rather than as a mechanism for loading modules.

To encourage modularity and usage of Workers in our codebase, we need a module loader.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 00:57:06 PDT
I am nearly certain that we can implement Jetpack's loader (minus sandboxing) for (chrome) workers on top of synchronous XMLHttpRequest.

See https://addons.mozilla.org/en-US/developers/docs/sdk/latest/modules/toolkit/loader.html for the API of Jetpack's loader.
Comment 2 Florian Bender 2013-05-15 01:00:31 PDT
To be clear, is this supposed to be something different from ES6 Modules / Loader API? 

If so, what is the difference between those APIs?

If not, why did you choose to do something different, i. e. what functionality are you missing from the spec'ed APIs?
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 02:55:03 PDT
It's not that ES6 modules are missing a functionality. It's that ES6 modules are not implemented and the JS team has their plate full with higher-priority stuff. I have been nagging them to prioritize modules, but if we can instead simply rely on a small module loader library, I can release that pressure.

In addition, implementing a Jetpack-compatible module loader means that work won't be lost once ES6 modules finally land.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-05-15 03:12:36 PDT
Sandboxing modules is pretty important no?  There's no way to implement that in workers today.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 03:15:51 PDT
I am quite satisfied with one Worker == one Sandbox, which is what we'll end up with.
Comment 6 Florian Bender 2013-05-15 03:34:43 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> It's not that ES6 modules are missing a functionality. It's that ES6 modules
> are not implemented […]
Alright, that's what I figured. 

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> In addition, implementing a Jetpack-compatible module loader means that work
> won't be lost once ES6 modules finally land.
Does this mean they kind of "shim" ES6 Modules (I thought they don't)? I. e. are Jetpack modules compatible with ES6 modules? Otherwise I don't get what you were saying. ;)
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 03:36:27 PDT
(In reply to Florian Bender from comment #6)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > In addition, implementing a Jetpack-compatible module loader means that work
> > won't be lost once ES6 modules finally land.
> Does this mean they kind of "shim" ES6 Modules (I thought they don't)? I. e.
> are Jetpack modules compatible with ES6 modules? Otherwise I don't get what
> you were saying. ;)

I'm just saying that I'll get Jetpack to adopt the feature :)
Comment 8 Florian Bender 2013-05-15 03:41:12 PDT
That's good enough :)
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-15 06:53:12 PDT
Created attachment 749861 [details] [diff] [review]
First prototype

Irakli, do you think we need something more sophisticated than this?
Comment 10 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2013-05-17 04:56:33 PDT
Comment on attachment 749861 [details] [diff] [review]
First prototype

Review of attachment 749861 [details] [diff] [review]:
-----------------------------------------------------------------

Hope this comments help.

::: toolkit/components/workerloader/loader.js
@@ +1,3 @@
> +(function(exports) {
> +  let buildModule = function(source, exports, require, module) {
> +    eval(source);

I believe that eval destroys JIT optimizations, so it may not be the best way to go about it. In
addition it will make all the scope and parent scope variables accessible overridable from module
source.


I wonder if something along this lines
would work better:

importScripts("data:application/javascript,require.define(module.path,function(exports, require, module){" +
              source +
              "\n})");

I also think that following has better JITing characteristics then then eval:

let buildModule = function(source) {
  return new Function("exports", "require", "module", source)
}

@@ +7,5 @@
> +    let modules = new Map();
> +
> +    return function require(path) {
> +      // Exports provided by the module
> +      let exports = {};

I'd suggest Object.create(null) instead.

@@ +12,5 @@
> +
> +      // Identification of the module
> +      let module = {
> +        id: path,
> +        uri: path

module.exports is required according to spec.

@@ +23,5 @@
> +      }
> +
> +      // Load source of module, synchronously
> +      let xhr = new XMLHttpRequest();
> +      xhr.open("GET", path, false);

I think you need some module path resolution logic.
According to specs and common standard there are two types of
require forms:

1. Absolute `require("foo/bar")` that means require
   module `bar` from package `foo`. In SDK and I believe
   devtools this would load code from
   `resource://gre/modules/commonjs/foo/bar.js`

   Although individual paths can be configured to be mapped
   to other locations (I don't know how relevant that would
   be in this context though).

2. Relative `require("../foo")` or `require("./bar")` both
   forms resolve relative to a requirer path. In this case
   I think it should be worker path for top level require
   and requirer module path for nested requirements.

Also note that use of `.js` file extensions is optional and
almost never used, so I'd suggest automatically adding file
extension if not present already.

@@ +29,5 @@
> +      xhr.send();
> +
> +      let source = xhr.responseText;
> +      try {
> +        // Isolate (somewhat) module construction

Not sure if it's a TODO or not but, each module needs
to get own `require` function so that `require("./foo")`
will be able to resolve path relative to requirer module's path.

@@ +37,5 @@
> +        // after all.
> +        modules.delete(path);
> +        throw ex;
> +      }
> +      return exports;

You should return module.exports instead since it's pretty common
to assign now exports to modules instead of populating exports.

I would also encourage freezing `module.exports` to forbid changes
to exports at runtime.
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-17 13:41:58 PDT
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #10)
> I think you need some module path resolution logic.
> According to specs and common standard there are two types of
> require forms:

Well, according to specs, module path resolution logic seems optional, so I figured it's probably not necessary for v1.

Also, I won't handle relative paths for the moment, as it doesn't really fit our usage scenario.
 
> Also note that use of `.js` file extensions is optional and
> almost never used, so I'd suggest automatically adding file
> extension if not present already.

ok
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-17 14:03:20 PDT
Created attachment 751208 [details] [diff] [review]
Module loader for workers

Ok, here's a prototype v2.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-05-19 19:59:57 PDT
I don't think this belongs in DOM anymore.
Comment 14 Nathan Froyd [:froydnj] 2013-05-20 08:31:36 PDT
Comment on attachment 751208 [details] [diff] [review]
Module loader for workers

Review of attachment 751208 [details] [diff] [review]:
-----------------------------------------------------------------

Seems OK to me, but I am not a JS expert, so take my opinion with a grain of salt.  Did you mean to use notamodule.xml in some test somewhere?  Is that the mysterious missing moduleF.js?

::: toolkit/components/workerloader/tests/worker_loading.js
@@ +50,5 @@
> +
> +// Test simple circular loading (moduleC.js and moduleD.js require each other)
> +add_test(function test_circular() {
> +  let C = require("moduleC.js");
> +  ok(true, "Loaded circular modules C and D");

Nit: I think you could have more descriptive names (module-circular-1, module-circular-2, as a strawman).

@@ +60,5 @@
> +
> +// Ensure that exceptions have the correct origin
> +add_test(function test_exceptions() {
> +  try {
> +    let E = require("moduleE.js");

Ditto on the descriptiveness.

@@ +68,5 @@
> +    isnot(ex.requireStack.indexOf("moduleE.js"), -1, "The name of the right file appears somewhere in the stack");
> +    is(ex.lineNumber, 7, "The error comes with the right line number");
> +  }
> +
> +  let F = require("moduleF.js");

You forgot to |hg add moduleF.js|?
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-22 08:15:39 PDT
Created attachment 752762 [details] [diff] [review]
Module loader for workers, v2

Here we go.
Irakli, do you want to review this? As I mentioned above, I would like to keep this first version simple, so no path resolution in a v1.
Comment 16 Nathan Froyd [:froydnj] 2013-05-23 08:19:43 PDT
Comment on attachment 752762 [details] [diff] [review]
Module loader for workers, v2

Review of attachment 752762 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly ok to me.

::: toolkit/components/workerloader/require.js
@@ +173,5 @@
> +          // There doesn't seem to be a better way to detect that the file couldn't be found
> +          throw new Error("Could not find module " + path);
> +        }
> +        // From the source, build a function and an object URL.
> +        // Note that this keeps line numbers but it messes file names.

Can you clarify this?  I suppose the line numbers are exactly those from the original source, since you don't insert newlines, but I don't know what "messes file names" is supposed to imply.

Might be a good idea to comment that the lack of newlines is important for correct line numbers.

@@ +209,5 @@
> +   *
> +   * @keys {string} The path to the module, prefixed with ":".
> +   * @values {function} A function wrapping the module.
> +   */
> +  require._tmpModules = Object.create(null);

Can we create _tmpModules in the same lexical scope as |require|, so we don't have any exposed data here?  (It looks like we ought to be able to, but there may be some JS semantics I am unaware of at work.)

::: toolkit/components/workerloader/tests/worker_test_loading.js
@@ +58,5 @@
> +
> +  exn = should_throw(() => require("moduleF-syntaxerror.xml"));
> +  ok(!!exn, "Attempting to load a non-well formatted module raises an error");
> +
> +  exn = should_throw(() => require("moduleE-throws-during-require.js"));

Picky nit: moduleE tests before moduleF tests.
Comment 17 Nathan Froyd [:froydnj] 2013-05-23 08:21:18 PDT
Comment on attachment 752762 [details] [diff] [review]
Module loader for workers, v2

Review of attachment 752762 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/workerloader/Makefile.in
@@ +11,5 @@
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +libs::
> +	$(NSINSTALL) $(srcdir)/require.js $(FINAL_TARGET)/modules/workers/

Mike, is there a better way to express this with build system variables?
Comment 18 Mike Hommey [:glandium] 2013-05-23 08:36:48 PDT
Comment on attachment 752762 [details] [diff] [review]
Module loader for workers, v2

Review of attachment 752762 [details] [diff] [review]:
-----------------------------------------------------------------

> > +
> > +include $(topsrcdir)/config/rules.mk
> > +
> > +libs::
> > +	$(NSINSTALL) $(srcdir)/require.js $(FINAL_TARGET)/modules/workers/
> 
> Mike, is there a better way to express this with build system variables?

There is, with INSTALL_TARGETS (see config/rules.mk)
Comment 19 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-23 11:02:16 PDT
(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 752762 [details] [diff] [review]
> Module loader for workers, v2
> 
> Review of attachment 752762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly ok to me.
> 
> ::: toolkit/components/workerloader/require.js
> @@ +173,5 @@
> > +          // There doesn't seem to be a better way to detect that the file couldn't be found
> > +          throw new Error("Could not find module " + path);
> > +        }
> > +        // From the source, build a function and an object URL.
> > +        // Note that this keeps line numbers but it messes file names.
> 
> Can you clarify this?  I suppose the line numbers are exactly those from the
> original source, since you don't insert newlines, but I don't know what
> "messes file names" is supposed to imply.
> 
> Might be a good idea to comment that the lack of newlines is important for
> correct line numbers.

Ok, clarified.

> @@ +209,5 @@
> > +   *
> > +   * @keys {string} The path to the module, prefixed with ":".
> > +   * @values {function} A function wrapping the module.
> > +   */
> > +  require._tmpModules = Object.create(null);
> 
> Can we create _tmpModules in the same lexical scope as |require|, so we
> don't have any exposed data here?  (It looks like we ought to be able to,
> but there may be some JS semantics I am unaware of at work.)

I couldn't find any way to do it: the script imported by |importScripts| is executed in the global scope, so it can only access exposed data.

> ::: toolkit/components/workerloader/tests/worker_test_loading.js
> @@ +58,5 @@
> > +
> > +  exn = should_throw(() => require("moduleF-syntaxerror.xml"));
> > +  ok(!!exn, "Attempting to load a non-well formatted module raises an error");
> > +
> > +  exn = should_throw(() => require("moduleE-throws-during-require.js"));
> 
> Picky nit: moduleE tests before moduleF tests.

Tsss.
Comment 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-05-23 11:38:03 PDT
Created attachment 753393 [details] [diff] [review]
Module loader for workers, v3

Updated comments, Makefile.in
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-06 06:21:38 PDT
Review ping?
Comment 22 (dormant account) 2013-06-06 11:05:43 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #21)
> Review ping?

bugzilla needs an api to send an irc ping too
Comment 23 Irakli Gozalishvili [:irakli] [:gozala] [@gozala] 2013-06-06 16:23:31 PDT
Comment on attachment 753393 [details] [diff] [review]
Module loader for workers, v3

Review of attachment 753393 [details] [diff] [review]:
-----------------------------------------------------------------

I'd still encourage to address few comments I've made since I believe that's
behavior users would expect. As of various makefiles I'm not really familiar
with our build system so I don't feel confident to providing any kind of feedback
so you may wanna ask someone else to look at that too.

r+ either way as you know better weather my suggestions make sense given the
problem space you're trying to solve.

::: toolkit/components/workerloader/require.js
@@ +184,5 @@
> +        let blob = new Blob([(new TextEncoder()).encode(source)]);
> +        objectURL = URL.createObjectURL(blob);
> +        paths.set(objectURL, path);
> +        importScripts(objectURL);
> +        require._tmpModules[name](exports, require, modules);

I still think you should not share require to allow relative require forms that are very common:

`require("./foo/bar")`, require("../bar")`

All you need is to capture `uri` and resolve to it. As a matter of fact I'd also encourage to implement
`require.main` which usually takes full `url` and all the modules then are resolved relative to it.

Both devtools and jetpack code now supports `require("foo/bar")` that resolves to `resource://gre/modules/commonjs/foo/bar.js`
I think it maybe a good idea to support that too. Note that neither node or any other commonjs implementations I'm aware of
treat `foo/bar` equivalent to `./foo/bar` quite the contrary, usually `foo/bar` is equivalent to `/foo/bar` where `/` is root
of your program.
Comment 24 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-07 06:07:05 PDT
I filed this as a followup bug 80664.
Comment 25 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-07 06:07:28 PDT
I meant bug 880664.
Comment 26 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-07 06:33:25 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=0cc18530ab31
Comment 27 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-10 05:30:12 PDT
Created attachment 760356 [details] [diff] [review]
Module loader for workers, v4

Same code, plus one additional argument check.
Comment 28 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-06-10 05:30:39 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=b547dd1cc5cb
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-06-10 08:26:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c99d5a4a0ff
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-06-10 12:50:40 PDT
https://hg.mozilla.org/mozilla-central/rev/9c99d5a4a0ff

Note You need to log in before you can comment on or make changes to this bug.