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

Upgrade Electron to v4 #3329

Merged
merged 15 commits into from
Dec 26, 2018
Merged

Upgrade Electron to v4 #3329

merged 15 commits into from
Dec 26, 2018

Conversation

juancampa
Copy link
Contributor

@juancampa juancampa commented Dec 15, 2018

Supersedes #3325

Talked to @rauchg and we think it's better to skip v3 altogether, go straight for v4, and use the canary channel to test it.

Sanity checks:

  • No errors on output
  • Plugins work (tested installing and using https://github.com/nyx-team/hyper-nyx)
  • Multiple tabs
  • Multiple windows
  • Splits
  • Menus
  • Auto update (pop up shows)
  • Clean shutdown
  • Vim session with splits
  • Clipboard works
    • Through menu
    • Hotkeys (broken but I added a work-around for now)
  • Initial zoom is the same as canary and pinch remains disabled
  • Performance is at least on par with electron v1
    • Throughput (2% slowdown, possibly due to different zoom levels)
    • Refresh rate
  • Sanity macOS
  • Sanity Windows
  • Went through all listed breaking changes

@juancampa juancampa mentioned this pull request Dec 15, 2018
16 tasks
@juancampa juancampa changed the title Upgrade Elecctron to v4 Upgrade Electron to v4 Dec 15, 2018
chabou
chabou previously requested changes Dec 16, 2018
app/auto-updater-linux.js Show resolved Hide resolved
app/commands.js Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
app/commands.js Show resolved Hide resolved
@chabou
Copy link
Contributor

chabou commented Dec 16, 2018

We have a problem with titlebar:
image

Transparent on top of Hyper


UPDATE: fixed 7fce112

@chabou chabou added the 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper label Dec 16, 2018
@juancampa
Copy link
Contributor Author

juancampa commented Dec 16, 2018

Hmm ok, seems like given that v3 worked fine but v4-beta has issues on Mac (and maybe windows?), it might be better to go with v3 for now as opposed to writing temporary work-arounds, since they might get fixed by the electron team eventually.

@rauchg, @chabou, thoughts on this?

@Stanzilla
Copy link
Contributor

4.0 is final as of today https://github.com/electron/electron/releases/tag/v4.0.0

package.json Outdated Show resolved Hide resolved
app/package.json Outdated Show resolved Hide resolved
@Stanzilla
Copy link
Contributor

That looks like electron-rebuild needs an update for electron v4 @MarshallOfSound

@juancampa
Copy link
Contributor Author

That looks like electron-rebuild needs an update for electron v4 @MarshallOfSound

@Stanzilla, It works now with the new node-abi https://github.com/lgeiger/node-abi/releases/tag/v2.5.1

@Stanzilla
Copy link
Contributor

ah, great, yarn was a bit annoying back in the day and would not upgrade that without electron-rebuild using it.

@juancampa
Copy link
Contributor Author

ah, great, yarn was a bit annoying back in the day and would not upgrade that without electron-rebuild using it.

Yeah, I struggled with that too. The trick is to delete node-abi from yarn.lock and have yarn figure out what the latest version is again.

Here's the yarn issue: yarnpkg/yarn#4986

Copy link
Contributor

@Stanzilla Stanzilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node version has to be updated for AppVeyor and CircleCI as well

@juancampa
Copy link
Contributor Author

juancampa commented Dec 23, 2018

node version has to be updated for AppVeyor and CircleCI as well

@Stanzilla, not sure what you mean here. Is there anything else other than these two lines?

https://github.com/zeit/hyper/pull/3329/files#diff-354f30a63fb0907d4ad57269548329e3R9
https://github.com/zeit/hyper/pull/3329/files#diff-180360612c6b8c4ed830919bbb4dd459R13

Following this table:

https://github.com/electron/node#branching-strategy

@Stanzilla
Copy link
Contributor

@juancampa nope that's fine, I just did not see the appveyor change in your PR. For Circle, I don't know how they specify node versions, I did not set up that CI.

@juancampa
Copy link
Contributor Author

@juancampa nope that's fine, I just did not see the appveyor change in your PR. For Circle, I don't know how they specify node versions, I did not set up that CI.

Looks like it's passing, so I'm not gonna worry about it for now

@juancampa juancampa dismissed Stanzilla’s stale review December 23, 2018 22:26

Seems to be passing

@juancampa
Copy link
Contributor Author

juancampa commented Dec 23, 2018

@Stanzilla, did you by any chance tested this branch on windows? I'm looking for someone to help me there, since I don't have a windows machine. A quick sanity check would be super helpful

@Stanzilla
Copy link
Contributor

Sure thing! It looks like you forgot to address https://github.com/electron/electron/blob/master/docs/api/breaking-changes.md#appmakesingleinstance and it errors because of it.

@juancampa
Copy link
Contributor Author

juancampa commented Dec 25, 2018

@Stanzilla sorry for the delay, I just had to back-merge canary. We reverted the commit that introduced that call. Feel free to test again when you get a few.

@Stanzilla
Copy link
Contributor

Stanzilla commented Dec 25, 2018

https://i.imgur.com/DH6D9I1.png
image
that should be https://github.com/electron/electron/blob/master/docs/api/breaking-changes.md#menu

Try looking at my initial electron v2 PR to see if you have all those changes covered #2967

@juancampa
Copy link
Contributor Author

juancampa commented Dec 25, 2018

@Stanzilla weird, I'm pretty sure I had that, since I did test the menus. I must have missed it in a rebase or merge. Should be good now

@Stanzilla
Copy link
Contributor

Does the config have a new location? it ignores what I do in ~/.hyper.js

@juancampa
Copy link
Contributor Author

Does the config have a new location? it ignores what I do in ~/.hyper.js

It doesn't. Seems to work fine on Linux and MacOS (I just changed the font size)

@Stanzilla
Copy link
Contributor

font size works, shell does not,

@juancampa
Copy link
Contributor Author

Yeah, I'm seeing it too, but it doesn't seem to work on the canary branch either so it must be something else

@Stanzilla
Copy link
Contributor

QA's been on a long vacation :D

@juancampa
Copy link
Contributor Author

@Stanzilla, Ok, I just looked into it and it was introduced by my optimistic session creation PR. Working on a fix

@Stanzilla
Copy link
Contributor

Sounds great!

@juancampa
Copy link
Contributor Author

juancampa commented Dec 25, 2018

Here's the fix on the shell issue: #3367. @Stanzilla let me know if everything worked fine on Windows so we can move forward with this PR

@rauchg rauchg merged commit dd68286 into vercel:canary Dec 26, 2018
@Stanzilla
Copy link
Contributor

well that was a quick and unexpected merge, but I can confirm that it works as expected now.

@juancampa
Copy link
Contributor Author

Optimistic merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants