Closed Bug 1386535 Opened 7 years ago Closed 7 years ago

Remove all add-on compatiblity shims

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Andy, we have a bunch of compatiblity shim that helps old bootstrapped addons which are using devtools API/modules via URLs/APIs that changed over time.
Do you think FF57 is the right release to drop all of that, or should we wait for 58 or some later step?

I also opened bug 1397452 which is specific to SDK features implemented in DevTools. I would have the same question about that.
Flags: needinfo?(amckay)
57 is fine, because all add-ons will need to be WebExtensions by then anyway. Likely the only add-ons that might be affected are ones looked after by the devtools team. So, do whatever is the most convenient for your team.
Flags: needinfo?(amckay)
Comment on attachment 8911804 [details]
Bug 1386535 - Remove all DevTools shims used to support legacy add-ons.

https://reviewboard.mozilla.org/r/183228/#review188474

Thanks, this looks good to me! :)
Attachment #8911804 - Flags: review?(jryans) → review+
The netmonitor and webconsole use a couple of these for Launchpad support [0]

  "devtools/client/framework/devtools": path.join(__dirname, "../../client/shims/devtools"),
  "devtools/shared/fronts/timeline": path.join(__dirname, "../../shared/shims/fronts/timeline"),

It seems kind of shim for these will be required to build those with webpack (I tested with the patch applied and `cd devtools/client/netmonitor/ && npm install && npm start` fails).

I'm not sure if this is something that could move into the launchpad repo or not. Consumers of launchpad are providing their own webpack config but it does look like there may be some kind of shared configuration in https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-launchpad/webpack.config.js. Julian, would it be possible / a good idea to move a couple of these shims into launchpad and make it work with all consumers?

[0]: https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools+%22%2Fshims%2F%22&redirect=true
Flags: needinfo?(jdescottes)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> The netmonitor and webconsole use a couple of these for Launchpad support [0]
> 
>   "devtools/client/framework/devtools": path.join(__dirname,
> "../../client/shims/devtools"),
>   "devtools/shared/fronts/timeline": path.join(__dirname,
> "../../shared/shims/fronts/timeline"),

That's so weird.
Can we map to the actual modules?!
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #5)
> > The netmonitor and webconsole use a couple of these for Launchpad support [0]
> > 
> >   "devtools/client/framework/devtools": path.join(__dirname,
> > "../../client/shims/devtools"),
> >   "devtools/shared/fronts/timeline": path.join(__dirname,
> > "../../shared/shims/fronts/timeline"),
> 
> That's so weird.
> Can we map to the actual modules?!

I'm not sure the details off hand, but this was probably done because the actual modules weren't de-chromed. And at least in the case of devtools.js there's not really a way to de-chrome it without going in and shimming/de-chroming a bunch of its imports. Since the require path is used in the tools, if there's no stub then webpack will go through and require the world and not build. There may be options available in the webpack config for this other than shims, I'm not sure.
I didn't realized client/shims/devtools and shared/shims/fronts/timeline were mock files just for webpack.
I no longer remove them in my last patch, it should prevent breaking webpack.
Also, referencing them in moz.build was a mistake, it shouldn't be included in firefox package!
Someone with more webpack expertize should followup on that to get rid of these shims folder completely,
as we should keep them just for webpack.

Also note that npm start doesn't work on console:
  ERROR in ../shared/components/reps/reps.js
  Module not found: Error: Can't resolve 'devtools/client/shared/vendor/lodash' in '/mnt/desktop/gecko/devtools/client/shared/components/reps'
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> Also note that npm start doesn't work on console:
>   ERROR in ../shared/components/reps/reps.js
>   Module not found: Error: Can't resolve
> 'devtools/client/shared/vendor/lodash' in
> '/mnt/desktop/gecko/devtools/client/shared/components/reps'
Nicolas just fixed that in bug 1403106

Honza
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6cc2df751ca9
Remove all DevTools shims used to support legacy add-ons. r=jryans
See Also: → 1403175
Just logged Bug 1403175 to cleanup & regroup our current webpack shims.

(In reply to Brian Grinstead [:bgrins] from comment #5)
> The netmonitor and webconsole use a couple of these for Launchpad support [0]
> 
>   "devtools/client/framework/devtools": path.join(__dirname,
> "../../client/shims/devtools"),
>   "devtools/shared/fronts/timeline": path.join(__dirname,
> "../../shared/shims/fronts/timeline"),
> 
> It seems kind of shim for these will be required to build those with webpack
> (I tested with the patch applied and `cd devtools/client/netmonitor/ && npm
> install && npm start` fails).
> 
> I'm not sure if this is something that could move into the launchpad repo or
> not. Consumers of launchpad are providing their own webpack config but it
> does look like there may be some kind of shared configuration in
> https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-
> launchpad/webpack.config.js. Julian, would it be possible / a good idea to
> move a couple of these shims into launchpad and make it work with all
> consumers?
> 
> [0]:
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Adevtools+%22%2Fshims%2F%22&redirect=true

Re: moving the shims to launchpad. I think we'll have a better picture once all the shims are in the same place but I tend to think that the shims should rather stay next to the code that consumes them. 

Imagine a console file uses a chrome privileged module. We have a shim for this module. for methods a() b() and c(). If the console file starts using a new method from the module, d(), it will break in launchpad. If the shim is in m-c it can easily be fixed. If the shim is provided by Launchpad then it means a new launchpad release is needed.
Flags: needinfo?(jdescottes)
https://hg.mozilla.org/mozilla-central/rev/6cc2df751ca9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.