1188401 - Make Loader.jsm less magical

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

While working on bug 1172010 I realized that Loader.jsm was quite magical.
It loads browser/devtools/main.js and automagically copy all its exported symbols to `devtools` being exposed by Loader.jsm.
I think it would export a `require` symbol from Loader.jsm, as that's the main usage of this module. Keep exporting `devtools` as being the result of require(browser/devtools/main) and may be expose something else to call devtools.main/devtools.reload.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Expose `require` out of Loader.jsm and use it!
Quite a massive patch as Loader.jsm is imported from everywhere.
I also tried to make Loader.jsm less magical by importing TargetFactory/Toolbox/Tools
from their path, instead of fetching them from `devtools` object.

I still need to go though all tests using devtools.require from their head.js.

This move allows to use `require` in most cases instead of `devtools.require`,
and hopefully is going to help make Loader.jsm less magic.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Let's see what says try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dee5c3b417f

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Should we export the main loader as `loader`, so you can avoid renaming at each usage (`let { devtools: loader } = ...`)?

If we do that, we may want to preserve the `devtools` export for compat with existing DevTools addons, so we would just offer `loader` as an alias to the same thing.

The nice property about this patch overall is you have both `loader` and `require` available when **calling** the loader, just like you would inside modules **loaded by** the loader.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

\o/ Try is green!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f80ea6a2365

(Note that this patch may only apply cleanly with bug 935366, 1181100 applied.)

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Should we export the main loader as `loader`, so you can avoid renaming at
> each usage (`let { devtools: loader } = ...`)?
> 
> The nice property about this patch overall is you have both `loader` and
> `require` available when **calling** the loader, just like you would inside
> modules **loaded by** the loader.

Yes, that's a good idea.
But I think exposing loader as a magic global was an error.
If we want to be closer to common js and/or simple web module loader,
we shouldn't have such magic global that comes from "somewhere".
It may be complex to switch to require.js or some other loader if we keep this magic global.

So I think it would be even better to refactor code using `loader` in order to require it explicitely.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8642531 [details] [diff] [review]
patch v3

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

f+ for now, I'd like to hear more details about `TargetFactory` first:

What's your reasoning for moving away from using the things `main.js` put on `devtools` via `devtools.X`, like `TargetFactory`, and requiring them specifically instead?  Do you think `main.js` should not put things on `devtools` at all?  Is it just that you find it cleaner this way?  Do intend to remove the `main` feature of the loader that imports these names onto `devtools`?  Just wondering what led you to do this type of change.

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > Should we export the main loader as `loader`, so you can avoid renaming at
> > each usage (`let { devtools: loader } = ...`)?
> > 
> > The nice property about this patch overall is you have both `loader` and
> > `require` available when **calling** the loader, just like you would inside
> > modules **loaded by** the loader.
> 
> Yes, that's a good idea.

Is it worth doing it now to get this simplification since you are already modifying lots of things anyway?  Not required, but it seems like a nice opportunity to do it.

> But I think exposing loader as a magic global was an error.
> If we want to be closer to common js and/or simple web module loader,
> we shouldn't have such magic global that comes from "somewhere".
> It may be complex to switch to require.js or some other loader if we keep
> this magic global.
> 
> So I think it would be even better to refactor code using `loader` in order
> to require it explicitely.

We do enough "other magic" with loading that I am not sure we're really that close to using some other loader...  Is there a reason we'd want to use some other loader, or is this just about being similar to other module loaders to reduce surprises?

Anyway, an explicit require does seem less surprising, I agree.

::: browser/devtools/animationinspector/animation-controller.js
@@ +18,1 @@
>                                   "devtools/toolkit/event-emitter");

Nit: fix indentation

@@ +20,1 @@
>                                   "devtools/server/actors/animation", true);

Nit: fix indentation

::: browser/devtools/canvasdebugger/test/head.js
@@ +22,5 @@
> +let { CanvasFront } = require("devtools/server/actors/canvas");
> +let { setTimeout } = require("sdk/timers");
> +let DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +let TiltGL = require("devtools/tilt/tilt-gl");
> +let {TargetFactory} = require("devtools/framework/target");

Nit: spaces in braces

@@ +23,5 @@
> +let { setTimeout } = require("sdk/timers");
> +let DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
> +let TiltGL = require("devtools/tilt/tilt-gl");
> +let {TargetFactory} = require("devtools/framework/target");
> +let {Toolbox} = require("devtools/framework/toolbox");

Nit: spaces in braces

::: browser/devtools/commandline/test/helpers.js
@@ +21,5 @@
>  var { helpers, assert } = (function() {
>  
>  var helpers = {};
>  
> +var { require} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

Nit: spaces in braces

::: browser/devtools/devtools-clhandler.js
@@ +43,4 @@
>        // Load the browser devtools main module as the loader's main module.
>        Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +      let hudservice = require("devtools/webconsole/hudservice");
> +      let { console }= Cu.import("resource://gre/modules/devtools/Console.jsm", {});

Nit: space after brace

::: browser/devtools/framework/gDevTools.jsm
@@ +9,5 @@
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +const {require, devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

Import devtools as loader to be consistent (or export as loader to start with as discussed in comments)

::: browser/devtools/framework/toolbox-process-window.js
@@ +7,5 @@
>  
>  let { gDevTools } = Cu.import("resource:///modules/devtools/gDevTools.jsm", {});
> +let { require } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +let { TargetFactory } = require("devtools/framework/target");
> +let {Toolbox} = require("devtools/framework/toolbox");

Nit: spaces in braces

::: browser/devtools/shared/widgets/Tooltip.js
@@ +22,2 @@
>  
> +loader.lazyRequireGetter(this, "beautify", "devtools/jsbeautify");

Do you have loader in scope here?  Wasn't sure.

::: toolkit/devtools/security/tests/unit/test_oob_cert_auth.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +let cert = require("devtools/toolkit/security/cert");

This may have been lazy because we need PSM initialized first...  though the test is already pretty intermittent...  Probably okay if try says so.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8642531 [details] [diff] [review]
>
> What's your reasoning for moving away from using the things `main.js` put on
> `devtools` via `devtools.X`, like `TargetFactory`, and requiring them
> specifically instead?

TargetFactory and Toolbox. This was just wrong. We are exporting some other module symbol from yet another module. We should just import them from the initial one.
Note that some smart people already started doing that ;)
  http://mxr.mozilla.org/mozilla-central/search?string=devtools%2Fframework%2Ftarget&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
(That's interesting to see dcamp introducing one of the first require while working on bug 855914, it was more obvious to pull it from target.js rather than main or anything else.)

> Do you think `main.js` should not put things on
> `devtools` at all?  Is it just that you find it cleaner this way?  Do intend
> to remove the `main` feature of the loader that imports these names onto
> `devtools`?  Just wondering what led you to do this type of change.

I'm fine having main.js exposing things, but only things of its own, not some other module exports.
The same applies to other symbols:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/main.js#15

Otherwise browser/devtools/main.js feels wrong today. It is entangled with gDevtools.jsm. I opened bug 1188405 in order to followup on that. gDevTools has to be moved to a commonjs module and it may be relevant to merge it with main.js... I do not have a clear plan yet, but something needs to be done around these two files.
For ex: gDevtools.jsm require(main.js) at its ends while main.js call gDevtools.jsm register method in its body...
I think my overall plan is to only have Loader.jsm as a jsm (and may be some other special one like console.jsm). So if we get rid of gDevTools, I think we may not have any more usage of loader.main(). we may still have main.js, but used like toolkit/devtools/server/main.js, via a regular require. It may be even easier to grok if we get rid of main module in loader. I don't think other commonjs loader have this pattern?
Anyway, let's continue this discussion in bug 1188405.

> (In reply to Alexandre Poirot [:ochameau] from comment #5)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > > Should we export the main loader as `loader`, so you can avoid renaming at
> > > each usage (`let { devtools: loader } = ...`)?
> > > 
> Is it worth doing it now to get this simplification since you are already
> modifying lots of things anyway?  Not required, but it seems like a nice
> opportunity to do it.

Yes, but in another patch, the current one is already painful enough!

> 
> > But I think exposing loader as a magic global was an error.
> > If we want to be closer to common js and/or simple web module loader,
> > we shouldn't have such magic global that comes from "somewhere".
> > It may be complex to switch to require.js or some other loader if we keep
> > this magic global.
> > 
> > So I think it would be even better to refactor code using `loader` in order
> > to require it explicitely.
> 
> We do enough "other magic" with loading that I am not sure we're really that
> close to using some other loader...  Is there a reason we'd want to use some
> other loader, or is this just about being similar to other module loaders to
> reduce surprises?

Yes, for these two reasons:
 - reduce surprise: someone coming from nodejs, require.js, or another other commonjs like modules system won't understand where does loader comes from. Irakli tried hard to convince everyone to not add any global in jetpack modules. And I believe he is very right.
 - compatiblity: I don't think we will switch to another loader for server modules anytime soon, but I think James want to do that soon for UI ones, like shared/widgets/! And if we want to use some existing module loader from the Web, we have to get rid of these magic globals. Also magic globals are going to be a pain if we happen to switch to harmony modules.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Comment on attachment 8642531 [details] [diff] [review]
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +22,2 @@
> >  
> > +loader.lazyRequireGetter(this, "beautify", "devtools/jsbeautify");
> 
> Do you have loader in scope here?  Wasn't sure.
> 

Yes, Tooltip is loader as commonjs module, via require().

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> > Comment on attachment 8642531 [details] [diff] [review]
> >
> > What's your reasoning for moving away from using the things `main.js` put on
> > `devtools` via `devtools.X`, like `TargetFactory`, and requiring them
> > specifically instead?
> 
> TargetFactory and Toolbox. This was just wrong. We are exporting some other
> module symbol from yet another module. We should just import them from the
> initial one.
> Note that some smart people already started doing that ;)
>  
> http://mxr.mozilla.org/mozilla-central/
> search?string=devtools%2Fframework%2Ftarget&find=&findi=&filter=^[^\0]*%24&hi
> tlimit=&tree=mozilla-central
> (That's interesting to see dcamp introducing one of the first require while
> working on bug 855914, it was more obvious to pull it from target.js rather
> than main or anything else.)

I think the original idea for browser/devtools/main.js was to export all the "main objects" about the front end as a convenience.  But of course, it's really just a random set of things, since who knows what you need in each case.

So anyway, I agree with you, it does seem better to require them explicitly from the thing that exports them.

> > Do you think `main.js` should not put things on
> > `devtools` at all?  Is it just that you find it cleaner this way?  Do intend
> > to remove the `main` feature of the loader that imports these names onto
> > `devtools`?  Just wondering what led you to do this type of change.
> 
> I'm fine having main.js exposing things, but only things of its own, not
> some other module exports.
> The same applies to other symbols:
>   http://mxr.mozilla.org/mozilla-central/source/browser/devtools/main.js#15

Right, that makes sense to me.  We may need to live with the existing main.js exports for add-ons, but should not add new ones.  Maybe using its exports should log a deprecation warning?

The lines in main.js that load up tools, themes, and register an observer make enough sense.  Anyway, I agree that the details depend on what happens to gDevTools in bug 1188405.

> > (In reply to Alexandre Poirot [:ochameau] from comment #5)
> > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > > > Should we export the main loader as `loader`, so you can avoid renaming at
> > > > each usage (`let { devtools: loader } = ...`)?
> > > > 
> > Is it worth doing it now to get this simplification since you are already
> > modifying lots of things anyway?  Not required, but it seems like a nice
> > opportunity to do it.
> 
> Yes, but in another patch, the current one is already painful enough!

I filed bug 1190857 for this.

> > > But I think exposing loader as a magic global was an error.
> > > If we want to be closer to common js and/or simple web module loader,
> > > we shouldn't have such magic global that comes from "somewhere".
> > > It may be complex to switch to require.js or some other loader if we keep
> > > this magic global.
> > > 
> > > So I think it would be even better to refactor code using `loader` in order
> > > to require it explicitely.
> > 
> > We do enough "other magic" with loading that I am not sure we're really that
> > close to using some other loader...  Is there a reason we'd want to use some
> > other loader, or is this just about being similar to other module loaders to
> > reduce surprises?
> Yes, for these two reasons:
>  - reduce surprise: someone coming from nodejs, require.js, or another other
> commonjs like modules system won't understand where does loader comes from.
> Irakli tried hard to convince everyone to not add any global in jetpack
> modules. And I believe he is very right.

Well, some of those systems also have their own random globals, like Node.js[1] with process and console.

Of course, that does not make it right just because someone else did it. ;)

I am not against requiring it explicitly, but if we just drop in something like:

   let loader = require("loader");

in every file that needs it, it could still be just as confusing for new people (as in "where is the magical loader module defined?").  I am not against such a change, but I want to make sure it really does solve a problem first, that's all.

[1]: https://iojs.org/api/globals.html

>  - compatiblity: I don't think we will switch to another loader for server
> modules anytime soon, but I think James want to do that soon for UI ones,
> like shared/widgets/! And if we want to use some existing module loader from
> the Web, we have to get rid of these magic globals. Also magic globals are
> going to be a pain if we happen to switch to harmony modules.

You're right that it's a problem for switching to a new generic loader from the web community or to ES2015 modules.

For the moment, it looks like James is planning to use another modified SDK loader (see bug 1180955), so I don't think these extra globals are blocking that work right now.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15ee7dbb53e

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> ::: toolkit/devtools/security/tests/unit/test_oob_cert_auth.js
> @@ +2,5 @@
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> >  "use strict";
> >  
> > +let cert = require("devtools/toolkit/security/cert");
> 
> This may have been lazy because we need PSM initialized first...  though the
> test is already pretty intermittent...  Probably okay if try says so.

I've requested many xpcshell tests on linux32 debug/opt and it seems to stay green.

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8643132 [details] [diff] [review]
patch v4

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

Hooray!  Probably the most files in one patch that I've reviewed so far... ;)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d3c48d49c692

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d3c48d49c692