1182194 - Remove usage of jsm files so we can reload

Status: RESOLVED DUPLICATE of bug 1525652

(DevTools :: General, enhancement)

Description

[James Long (:jlongster)]

This might be a lot of work but we need a bug for it. In order to reload our tools, we need to avoid using `Cu.import` anywhere, which involves not using .jsm files. I don't see any major barriers for doing this, just a lot of grunt work to clean up our files.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

A couple of random thoughts:
How far do we go with this e.g. does this mean moving Services.jsm to a js file and then using require?

It would also be great to always use lazy getters but then we couldn't deconstruct values:
e.g. let {x, y, z} = Cu.Import("path/blah.jsm");

Comment 2

[James Long (:jlongster)]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> A couple of random thoughts:
> How far do we go with this e.g. does this mean moving Services.jsm to a js
> file and then using require?

I doubt it, for special cases like that every module just needs to make sure that if they mutate a service in any way, they "unmutate" it when being reloaded. For example adding an event handler on the Observer service. I already ran into this when playing with loaders: promise and event-emitter don't unregister themselves from the observer service, but it shouldn't be hard to do that.

These services are global C++ singleton services (as far as I know) so we'll always have to deal with them like that.

> It would also be great to always use lazy getters but then we couldn't
> deconstruct values:
> e.g. let {x, y, z} = Cu.Import("path/blah.jsm");

Yeah, I'm not convinced that we really need to lazy load that much. We shouldn't ever need to lazy load shared files, as they are almost always needed immediately anyway. The only times we should lazy load in my opinion is when a tool tried to access a different tool (like the console loads a location in the debugger), the console shouldn't load the whole debugger.

Comment 3

[James Long (:jlongster)]

Thinking about it more, I guess there are several places where it's important to lazy-load, but Jordan is working through the loader APIs to make that easy, so we'll figure that out.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Note that JSMs can be unloaded too, but we would still prefer a more common API (especially outside Mozilla) like require():

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.unload

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Use DevToolsUtils module instead of JSM

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Remove DevToolsUtils.jsm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8893f500dc03
I kept that out of the refactoring patch, as we may want to keep it for addons??
But I ran on try with this patch to see if that pass without it!

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8639766 [details] [diff] [review]
Use DevToolsUtils module instead of JSM

This patch is really naive.
Replace Cu.import(DevToolsUtils.jsm) by require(DevToolsUtils.js)
with some related cleanup here and there.

Note that there is no need to lazy load some modules if:
- they are already loaded (Loader.jsm)
- they are going to be used by the related module no matter what (DevtoolsUtils in some cases)

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8639767 [details] [diff] [review]
Remove DevToolsUtils.jsm

This one is more at risk as it's not clear if any addon is using this module or not.

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8639767 [details] [diff] [review]
> Remove DevToolsUtils.jsm
> 
> This one is more at risk as it's not clear if any addon is using this module
> or not.

If I look at this, there is only two addons using it on AMO:
https://mxr.mozilla.org/addons/search?string=DevToolsUtils.jsm&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
  ICS Inspector
  Remote Developer Tools Server
Both written by
  Philipp Kewisch

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 8639766 [details] [diff] [review]
Use DevToolsUtils module instead of JSM

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

My opinion is that we should avoid devtools.require and define a require const instead to make it easier to move to an html5 module system in the future.
I wouldn't r- this for that though.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5f81d6f390c5
https://hg.mozilla.org/integration/fx-team/rev/825a6e29bff1

Comment 12

[Alexandre Poirot [:ochameau]]

Philipp, this is just to let you know that your two addons may break with upcoming Fx release.
You would just need to replace:
  Cu.import("resource://gre/modules/devtools/DevToolsUtils.jsm");
By:
  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
  let DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils");

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> A couple of random thoughts:
> How far do we go with this e.g. does this mean moving Services.jsm to a js
> file and then using require?

My initiative here is to convert most *devtools* JSM,
that to ensure that `tools srcdir`/`tools reload` gcli commands, correctly reload devtools from sources. We plan to eventually have devtools sources in a distinct repo.
So that we only care about devtools modules, the others are going to be pulled out of Firefox resources.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #10)
> Comment on attachment 8639766 [details] [diff] [review]
> Use DevToolsUtils module instead of JSM
> 
> Review of attachment 8639766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My opinion is that we should avoid devtools.require and define a require
> const instead to make it easier to move to an html5 module system in the
> future.

Sounds like a separate issue. I do have idea that would ease that and could be another refactoring.
See bug 1172010 comment 2. In most cases I just reuse devtools.require, like other usage in the same file. I filled bug 1188401.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Convert LayoutHelpers.jsm to a module

Similar move but for LayoutHelpers.jsm.

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

Comment 16

[James Long (:jlongster)]

Thanks for working on this! re-assigning to you for now. I was going to audit our usage of it soon but it's great your actually landing changes.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks for the note. I'll make sure to change this. From what I have been reading I believe the new way to import is backwards compatible, in case not please let me know.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/5f81d6f390c5
https://hg.mozilla.org/mozilla-central/rev/825a6e29bff1

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Convert LayoutHelpers.jsm into a commonjs module

This time, with a green try.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8c874cde522

I removed the JSM in this patch as it sounds even less used than DevToolsUtils.
But actually, two addons are using it:
https://mxr.mozilla.org/addons/search?string=LayoutHelpers.jsm
  DevTools Tweaks by Luke Bryan
    (Pinged here about this - https://github.com/programmin1/DevTools-Tweaks/issues/6)
  YouTube Animated Thumbnails by victor
    (Will ni? him)

Comment 20

[Alexandre Poirot [:ochameau]]

Victor, please ack the upcoming breakage for your "YouTube Animated Thumbnails" addon.

Comment 21

[Alexandre Poirot [:ochameau]]

I'm going to handle SourceMap.jsm in existing bug 935366.

Comment 22

[Victor Porof [:vporof][:vp]]

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Victor, please ack the upcoming breakage for your "YouTube Animated
> Thumbnails" addon.

Ugh, ok.

Comment 23

[Pulsebot]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/297f233ab9d5

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ochameau, maybe it's time to close this bug?

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ochameau, maybe it's time to close this bug?

Comment 28

[Alexandre Poirot [:ochameau]]

No, there is still work to be done here. I should have landed these patches in individual bugs...

Comment 29

[Alexandre Poirot [:ochameau]]

Bug 1525652 is going to migrate all jsm to ESM.

Comment 30

[:Gijs (he/him)]

(In reply to Alexandre Poirot [:ochameau] from comment #29)

Bug 1525652 is going to migrate all jsm to ESM.

*** This bug has been marked as a duplicate of bug 1525652 ***

Is that actually going to allow us to live reload? I kind of expect it won't...

Comment 31

[Alexandre Poirot [:ochameau]]

(In reply to :Gijs (he/him) from comment #30)

(In reply to Alexandre Poirot [:ochameau] from comment #29)

Bug 1525652 is going to migrate all jsm to ESM.

*** This bug has been marked as a duplicate of bug 1525652 ***

Is that actually going to allow us to live reload? I kind of expect it won't...

It won't. But we no longer pursue live reloading devtools for a little while.