1384527 - Stop using Promise.jsm from DevTools

Status: NEW

(DevTools :: General, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

We still import Promise.jsm from DevTools:
http://searchfox.org/mozilla-central/search?q=Promise.jsm%22&case=false&regexp=false&path=devtools%2F

Bug 1378173 aims at removing it from tree.

We already have bug 1233890 and bug 1233891, which are about stop using require("promise") and use Promise global instead.
This involves more refactoring than just mapping require("promise") to DOM Promise.

It looks like we could just do that first:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c132af40db995df0f3533baa21e984e68ff7365d&selectedJob=117999510

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - Remove Promise.jsm usages from DevTools.

Review commit: https://reviewboard.mozilla.org/r/161444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161444/

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - Stop importing event-emitter as a JSM.

Review commit: https://reviewboard.mozilla.org/r/161446/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161446/

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1384527 - remove Promise.jsm from Loader.jsm.

Review commit: https://reviewboard.mozilla.org/r/161448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161448/

Comment 4

[Alexandre Poirot [:ochameau]]

Oh no, I've been completely fooled as I missed actually replacing require(promise) with DOM Promise.
So we do have to manually remove all the calls to require(promise).
My two first patches are still actionnable, it removes all direct usages of Promise.jsm.
The last one will only be landable once we removed all the require(promise).

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8890314 [details]
Bug 1384527 - Stop importing event-emitter as a JSM.

This change should also remove:
  devtools/client/inspector/webpack/rewrite-event-emitter.js
and its reference from:
  devtools/client/inspector/webpack.config.js
  loaders: [path.join(__dirname, "./webpack/rewrite-event-emitter")],

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8890314 [details]
Bug 1384527 - Stop importing event-emitter as a JSM.

Moved to bug 1386299

Comment 7

[Alexandre Poirot [:ochameau]]

Here is my plan:

0/ bug 1387122: Remove all already useless import of promise with capital P: Promise = require("promise")

1/ bug 1387123: Replace all usage of require("promise").defer by require("devtools/shared/defer").defer

Why? It can be automated! Ideally we would look into each usage and try to see if we can replace each with new Promise(),
that's still something we could look into bug 1233890 and bug 1233891. Some usage of defer are going to be hard to refactor to pure Promise, so I imagine we will end up with a few usages of devtools/shared/defer anyway.

2/ bug 1387128: Drop all promise = require("promise") and replace promise by Promise

Thanks to step 1, we should no longer use promise.defer.
All the other usages of Promise.jsm should be supported by DOM Promise: new constructor, resolve, reject, race, all.
i.e. if I'm not mistaken, defer was the only things Promise.jsm implements that DOM Promise doesn't.

3/ Go manually fix the couple of custom promise import like:
   const { resolve } = require("promise")
   const {defer} = require("promise")

I imagine we can do this last step in this bug.

Hopefully all that can be mostly automated and I haven't missed a critical usage preventing this plan from happening?!

Comment 8

[Alexandre Poirot [:ochameau]]

Gabriel, It is unclear to me what is unclear to you?

The main motivation here, to finally get rid of Promise.jsm, is bug 1378173.
Promise.jsm is planed to be removed from mozilla-central, so we should stop using it on devtools side.

Why is it being removed? Because of DOM Promises which replaced this.
And I think it introduces extra slowness related to cross compartment wrappers.

Regarding bug 1386299 and the JSM boilerplate removal,
getting rid of JSM for *our* modules is another story.
This isn't a goal here.
It just happened that I could drop Promise.jsm usage in the boilerplace
by converting event-emitter.js to a a commonjs-module only.

bug 1182194 is focusing on the JSM removal.
I did removed all the JSM from /devtools/server a long time ago,
we should still aim to convert JSM to regular modules also in client and shared.
Why?
* it is much easier to support unload/reload if you get rid of JSM
You can see that when using the add-on workflow or if you have to ship devtools as an add-on.
* it would better work in launchpad, with tools loaded in a tab.
* as for Promise/Promise.jsm, it simplifies our codebase by using only one module system.

Is everything crystal clear with this additonal info?

Comment 9

[Gabriel [:gl] (ΦωΦ)]

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Gabriel, It is unclear to me what is unclear to you?
> 
> The main motivation here, to finally get rid of Promise.jsm, is bug 1378173.
> Promise.jsm is planed to be removed from mozilla-central, so we should stop
> using it on devtools side.
> 
> Why is it being removed? Because of DOM Promises which replaced this.
> And I think it introduces extra slowness related to cross compartment
> wrappers.
> 
> Regarding bug 1386299 and the JSM boilerplate removal,
> getting rid of JSM for *our* modules is another story.
> This isn't a goal here.
> It just happened that I could drop Promise.jsm usage in the boilerplace
> by converting event-emitter.js to a a commonjs-module only.
> 
> bug 1182194 is focusing on the JSM removal.
> I did removed all the JSM from /devtools/server a long time ago,
> we should still aim to convert JSM to regular modules also in client and
> shared.
> Why?
> * it is much easier to support unload/reload if you get rid of JSM
> You can see that when using the add-on workflow or if you have to ship
> devtools as an add-on.
> * it would better work in launchpad, with tools loaded in a tab.
> * as for Promise/Promise.jsm, it simplifies our codebase by using only one
> module system.
> 
> Is everything crystal clear with this additonal info?

I think there might be some misunderstanding. I am clear of what we are trying to do in this particular bug, but I am looking to see if there is a list of what we are trying to deprecate/not-to-use-anymore so that we can keep these things in mind when it comes to reviewing new patches while this work is happening.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #9)
> I think there might be some misunderstanding. I am clear of what we are
> trying to do in this particular bug, but I am looking to see if there is a
> list of what we are trying to deprecate/not-to-use-anymore so that we can
> keep these things in mind when it comes to reviewing new patches while this
> work is happening.

Ah ok.
Not that I am aware of.

Sole, do we have any page with a list of deprecate/not-to-use-anymore?

Regarding promises, once the removal is going to be complete, the module
will disappear so you won't be able to use it anymore.

And JSM, I would be really surprised if someone in the team creates a new one ;)

Comment 11

[Soledad Penades [:sole] [:spenades]]

Bonjour, mes amis!

On the NoSDK side, we have a list of replacements and advice of how to replace: https://github.com/devtools-html/snippets-for-removing-the-sdk but that's about it.

That doesn't stop us from creating a shared list (perhaps a spreadsheet?) where we list what we're up to very succinctly. Then it's a matter of doing an in-page search before you go and change something. Maybe.

Another option would be to have a thread in our email list letting people know what you intend to remove/replace and gathering input.

I'm happy with any, the last one might have more noise but inform more than having just a spreadsheet that doesn't "emit pings" :-)