Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update electron to v16 #3355

Closed
wants to merge 1 commit into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jan 24, 2022

Summary

I'm looking into improving hermes chrome devtools integration and integrating the CPU profiler. While looking into it I found that I need to integrate the node version of chrome devtools using devtools://devtools/bundled/js_app.html instead of chrome-devtools://devtools/bundled/inspector.html. However the profiler tab was blank, while it was working fine in chrome. I figured this is probably because of using an older version of electron since it uses the bundled devtools.

This updates electron to v16 which brings an updated version of bundled devtools and fixes the issue. After the update the Profile tab in the hermes debugger plugin works fine (well it appears, it still isn't integrated with hermes).

The main breaking change was the removal of the remote module, it is now part of @election/remote. I followed the migration instructions here https://github.com/electron/remote

Changelog

Update electron to v16

Test Plan

Basic test of the main flipper functionality to make sure it still works.

Before

Old devtools url, lots of browser specific stuff

image

New devtools url, clean but broken profiler tab

image

After

New devtools url, working profiler tab

image

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 24, 2022
@coveralls
Copy link

coveralls commented Jan 24, 2022

Pull Request Test Coverage Report for Build 1878027283

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 50.604%

Files with Coverage Reduction New Missed Lines %
desktop/test-utils/src/pathUtils.tsx 2 57.14%
Totals Coverage Status
Change from base Build 1876681370: 0.03%
Covered Lines: 7424
Relevant Lines: 13260

💛 - Coveralls

@passy
Copy link
Member

passy commented Feb 1, 2022

Thanks for giving this a go. We've not been able to upgrade Electron for a while because of severe slowdowns when calling out to child processes. We're currently working on removing Electron entirely as this bug, last time I checked, still existed and makes Flipper pretty much unusable.

@janicduplessis
Copy link
Contributor Author

I see, maybe it would be possible to bundle a copy of the chrome devtools for the hermes debugger instead of using the electron built in ones? Would that be a good solution?

@passy
Copy link
Member

passy commented Feb 11, 2022

I see, maybe it would be possible to bundle a copy of the chrome devtools for the hermes debugger instead of using the electron built in ones? Would that be a good solution?

That sounds like a great idea. Is that something you'd be interested in looking into?

@janicduplessis
Copy link
Contributor Author

@passy Cool, yes I can have a look at this :)

@martinbigio
Copy link

@janicduplessis did you get a chance to work on this?

@janicduplessis
Copy link
Contributor Author

Hey @martinbigio!

I had a look at publishing chrome devtools in a way that can be consumed by electron here https://github.com/janicduplessis/chrome-devtools-frontend-prebuilt. https://github.com/ChromeDevTools/devtools-frontend is published to npm, but needs to be built so it is not really possible to use directly.

My main concern with this approach is that devtools is relatively large, the built size is around 100mb. It might be possible to strip some of it that we don't use but haven't had a look at that.

I was also curious about what the exact perf issues with updating electron were, since this might still end up being the best solution.

I don't really have time to work on that more for now so feel free to pick this up, or explore other possible solutions :)

@janicduplessis
Copy link
Contributor Author

Looks like latest version updated electron and devtools profiler tab now works fine!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants