Open
Bug 1384527
Opened 4 years ago
Updated 4 months ago
Stop using Promise.jsm from DevTools
Categories
(DevTools :: General, enhancement, P2)
DevTools
General
Tracking
(firefox57 fix-optional)
NEW
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)
59 bytes,
text/x-review-board-request
|
Details |
We still import Promise.jsm from DevTools: http://searchfox.org/mozilla-central/search?q=Promise.jsm%22&case=false®exp=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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•4 years ago
|
||
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).
Assignee | ||
Comment 5•4 years ago
|
||
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")],
Assignee | ||
Comment 6•4 years ago
|
||
Comment on attachment 8890314 [details] Bug 1384527 - Stop importing event-emitter as a JSM. Moved to bug 1386299
Assignee | ||
Comment 7•4 years ago
|
||
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?!
Assignee | ||
Updated•4 years ago
|
Attachment #8890313 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8890314 -
Attachment is obsolete: true
Assignee | ||
Comment 8•4 years ago
|
||
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)
Comment 9•4 years ago
|
||
(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)
Assignee | ||
Comment 10•4 years ago
|
||
(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)
Comment 11•4 years ago
|
||
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)
Updated•4 years ago
|
Priority: -- → P2
Updated•4 years ago
|
status-firefox57:
--- → fix-optional
Blocks: 1439048
Blocks: dt-polish-debt
Updated•3 years ago
|
Blocks: dt-fission
No longer blocks: 1439048
Updated•3 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•2 years ago
|
No longer blocks: dt-fission
You need to log in
before you can comment on or make changes to this bug.
Description
•