-
Notifications
You must be signed in to change notification settings - Fork 953
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
Update electron to v16 #3355
Conversation
Pull Request Test Coverage Report for Build 1878027283
💛 - Coveralls |
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. |
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? |
@passy Cool, yes I can have a look at this :) |
2c540b1
to
c6ca156
Compare
@janicduplessis did you get a chance to work on this? |
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 :) |
Looks like latest version updated electron and devtools profiler tab now works fine! |
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 ofchrome-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/remoteChangelog
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
New devtools url, clean but broken profiler tab
After
New devtools url, working profiler tab