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.

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.

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?

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.

Sandboxing modules is pretty important no? There's no way to implement that in workers today.

I am quite satisfied with one Worker == one Sandbox, which is what we'll end up with.

(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. ;)

(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 :)

That's good enough :)

Irakli, do you think we need something more sophisticated than this?

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.

(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

Ok, here's a prototype v2.

I don't think this belongs in DOM anymore.

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|?

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 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 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 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)

(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.

Updated comments, Makefile.in

Review ping?

(In reply to David Rajchenbach Teller [:Yoric] from comment #21) > Review ping? bugzilla needs an api to send an irc ping too

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.

I filed this as a followup bug 80664.

I meant bug 880664.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0cc18530ab31

Same code, plus one additional argument check.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b547dd1cc5cb