Closed
Bug 1305669
Opened 8 years ago
Closed 8 years ago
Migrate from Cu.import to CommonJS
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: Yoric, Unassigned)
References
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.
Reporter | ||
Comment 1•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Summary: Migrate from Cu.import to RequireJS → Migrate from Cu.import to CommonJS
Reporter | ||
Comment 2•8 years ago
|
||
(as remarked on the thread, I wrote "RequireJS" but I meant "CommonJS")
Comment 3•8 years ago
|
||
Why commonjs vs. import() function that is an ECMA proposal?
Comment 4•8 years ago
|
||
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)
Reporter | ||
Comment 5•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(jcoppeard)
Flags: needinfo?(gandalf)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(wmccloskey)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(jorendorff)
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
(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)
Comment 9•8 years ago
|
||
(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)
Comment 10•8 years ago
|
||
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.
Reporter | ||
Comment 11•8 years ago
|
||
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
Closed: 8 years ago
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gandalf)
Resolution: --- → WONTFIX
Comment 12•8 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)
Reporter | ||
Comment 13•8 years ago
|
||
Flags: needinfo?(dteller)
Reporter | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•