Open Bug 1384527 Opened 2 years ago Updated 6 months ago

Stop using Promise.jsm from DevTools

Categories

(DevTools :: General, enhancement, P2)

enhancement

Tracking

(firefox57 fix-optional)

Tracking Status
firefox57 --- fix-optional

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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
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).
Depends on: 1233890, 1233891
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")],
Depends on: 1386299
Comment on attachment 8890314 [details]
Bug 1384527 - Stop importing event-emitter as a JSM.

Moved to bug 1386299
Depends on: 1387122
Depends on: 1387123
Depends on: 1387128
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?!
Attachment #8890313 - Attachment is obsolete: true
Attachment #8890314 - Attachment is obsolete: true
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?
Flags: needinfo?(gl)
Depends on: 1388054
(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.
Flags: needinfo?(gl)
(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 ;)
Flags: needinfo?(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" :-)
Flags: needinfo?(spenades)
Depends on: 1388364
Priority: -- → P2
No longer blocks: 1439048
Depends on: 1454373
Product: Firefox → DevTools
No longer blocks: dt-fission
Blocks: 1180427
You need to log in before you can comment on or make changes to this bug.