Migrate from Cu.import to CommonJS

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: Yoric, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Based on the following conversation on dev-platform: https://groups.google.com/d/msg/mozilla.dev.platform/JhmCjt3fnP0/10RRHJtvBAAJ

Migrating away from Cu.import and to a more standard module system would be useful to: 1/ save lots of memory; 2/ make our code much more friendly to static analysis. One of the possibilities is to move to RequireJS modules. Opening this bug to follow this idea, other bugs might be opened for other ideas.
Even if we move to RequireJS modules, we still need to keep a version of `Cu.import` to avoid breaking Thunderbird and add-ons. Maintaining this version of `Cu.import` should also help with migrating some corner case modules, tests, etc.

The following steps are designed so that we can land them one at a time:


1/ Progressively move from `EXPORTED_SYMBOLS` to `exports`

a. Ensure our .jsm don't define a global `exports` or `this.exports`. I believe that the easiest way to do this is to patch `mozJSComponentLoader` to detect such globals and MOZ_ASSERT(false). Rewrite code that violates this invariant.

b. Patch up `mozJSComponentLoader` to inject a global `exports` in loaded files.

c. Patch up `mozJSComponentLoader` to use `exports` instead of `EXPORTED_SYMBOLS` if available. 

d. Code a rewriting script to migrate as many jsm, tests as possible to use `exports` automatically.

e. Start to migrate manually code that hasn't migrated yet.

x. Eventually, at least in Firefox, start printing warnings for code that defines `EXPORTED_SYMBOLS`/`this.EXPORTED_SYMBOLS`.



2/ Collapse mozilla-central compartments (not add-on compartments)

a. Ensure our .jsm don't overwrite properties of globals (Object, String, ...). I'm not exactly sure how we can guarantee it, but I suspect that we can MOZ_ASSERT this in one of our many wrappers. Also, once we have static analysis running on JS, this can be moved to one of our linters.

b. Patch up `mozJSComponentLoader` to always reuse the same compartment when there is no add-on id (optionally, we could also collapse to one compartment per add-on id, but I'm not sure it's worth the trouble). At this stage, in a module, code executed upon load by the `mozJSComponentLoader` is executed with the following semantics:

var initializer = new Function("exports", sourceCode);
var exports = {};
var self = { exports: exports };
initializer.bind(self)(exports);
Object.freeze(exports);

Note: at this stage, we still need a way to access BackstagePass.



3/ Progressively move from `Cu.import` to `require`.

Note: This does not cover DevTools/Jetpack code, which relies upon an implementation of `require`. We probably should not touch this code. I assume that it's easy to make the difference.

Note 2: I have found exactly 2 uses of `Cu.unload` in our code, I hope that we can rewrite around them.

a. Ensure our .jsm don't define a global `require` or `this.require`. I believe that the easiest way to do this is to patch `mozJSComponentLoader` to detect such globals and MOZ_ASSERT(false). Rewrite code that violates this invariant. 

b. Patch up `mozJSComponentLoader` so that `Cu.import` auto-exports code published through `EXPORTED_SYMBOLS` but not exports.

c. Patch up `mozJSComponentLoader` to introduce a global function `require` with the following semantics:

function require(url) {
  Cu.import(url).exports
}

d. Code a rewriting script to migrate as many jsm, tests as possible to use `require` automatically.

e. Start to migrate manually code that hasn't migrated yet.

x. Eventually, at least in Firefox, start printing warnings for code that uses `Cu.import`.


===


Bobby, what do you think of this plan? Will you be available to mentor/review patches to `mozJSComponentLoader`?
Flags: needinfo?(bobbyholley)
Summary: Migrate from Cu.import to RequireJS → Migrate from Cu.import to CommonJS
(as remarked on the thread, I wrote "RequireJS" but I meant "CommonJS")
Why commonjs vs. import() function that is an ECMA proposal?
Realistically I don't have the cycles to get involved in this and advocate for any particular solution right now. Here are the steps I'd propose:
(1) Develop a consensus proposal that has the support of JS people (jorendorff), fx-team technical leadership, and probably bz and billm for good measure. I don't get the sense from the thread that we have a consensus yet.
(2) Ask blake if he has cycles to review the patches. If not, figure out what review load you're looking for and I'll see what timeline I can do it in.
Flags: needinfo?(bobbyholley)
For reference, there are now two pre-RFCs on the topic.

* migration towards ES6 modules: https://gist.github.com/Yoric/2a7c8395377c7187ebf02219980b6f4d
* migration towards CommonJS modules: https://gist.github.com/Yoric/777effee02d6788d3abc639c82ff4488
Flags: needinfo?(jcoppeard)
Flags: needinfo?(gandalf)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jorendorff)
jonco came up with an approach that should work for migrating to ES modules:

* Make `Cu.import()` support ES modules as well as JSMs (maybe based on file extension).

* Make `Cu.import()` always load all ES modules in a single shared chrome global. Within this global, ES `import` can be used to import other ES modules; and of course `Cu.import()` can still be used to import JSMs.

No other chrome globals will have ES modules, for now.
Flags: needinfo?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> jonco came up with an approach that should work for migrating to ES modules:
> 
> * Make `Cu.import()` support ES modules as well as JSMs (maybe based on file
> extension).

Isn't there a more robust solution? I was expecting that the `export` could somehow be detected. Otherwise, I would assume that a JSM is any chrome file loaded through `Cu.import()` that defines `EXPORTED_SYMBOLS`.

> 
> * Make `Cu.import()` always load all ES modules in a single shared chrome
> global. Within this global, ES `import` can be used to import other ES
> modules; and of course `Cu.import()` can still be used to import JSMs.
> 
> No other chrome globals will have ES modules, for now.

So far, that looks a lot like my proposal, so I'm of course happy :)
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> jonco came up with an approach that should work for migrating to ES modules:
> 
> * Make `Cu.import()` support ES modules as well as JSMs (maybe based on file
> extension).
> 
> * Make `Cu.import()` always load all ES modules in a single shared chrome
> global. Within this global, ES `import` can be used to import other ES
> modules; and of course `Cu.import()` can still be used to import JSMs.
> 
> No other chrome globals will have ES modules, for now.

I think this proposal makes sense. We can use a BackstagePass for the global of all Cu.imported ES modules.

However, we'll need to be careful to ensure that Cu.import of old .jsms works properly within ES modules. It needs to add bindings to the ES6 module scope rather than the global. Also, we'll need to decide what the new-style Cu.import should return. Probably just an object whose properties are the module exports.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #8)
> Also, we'll need to decide what the new-style
> Cu.import should return. Probably just an object whose properties are the
> module exports.

Yes, it should return the module namespace object I think.
Flags: needinfo?(jcoppeard)
We won't be able to do the migration until we've fixed the performance problems with our current require() implementation, see bug 1308332. The performance differences between Cu.import and require() are striking.
We have been working with Jorendorff and Jonco on a migration path that brings us straight to ES6 modules, so closing off this bug. I imagine that we'll also want to port `require()` to ES6 modules, too, but that's a different story altogether.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gandalf)
Resolution: --- → WONTFIX

Comment 12

2 years ago
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #11)
> We have been working with Jorendorff and Jonco on a migration path that
> brings us straight to ES6 modules, so closing off this bug.

Is there another bug to follow?
Flags: needinfo?(dteller)
(In reply to David Bruant from comment #12)
> Is there another bug to follow?

Filed as bug 1308512.
Flags: needinfo?(dteller)
See Also: → bug 1308512
You need to log in before you can comment on or make changes to this bug.