[WIP] Upgrade Electron version to 5.0.6 #781

kanishk98 · 2019-06-28T21:35:33Z

What's this PR do?

Makes a couple Electron version leaps to 5.0.6.

Any background context you want to provide?

Needed to enable webviewTag when creating a BrowserWindow in the main process.
Still have to assure ourselves that things haven't broken in the upgrade.

You have tested this PR on:

Windows
Linux/Ubuntu
macOS

vsvipul · 2019-06-28T21:51:11Z

Why are we upgrading directly to v5? Didn't we agree to do incremental upgrades for electron?

kanishk98 · 2019-06-28T21:53:25Z

Yeah, but Akash and I figured it might be better to upgrade directly to the highest version where things don't break outright, and then work downward from there.

akashnimare · 2019-06-29T09:52:05Z

This is still WIP and we still need to test a lot of stuff before merging this. Let me just check if the auto-updates works with this version. @kanishk98 can you check the other features/settings etc and make sure everything works as expected.

kanishk98 · 2019-06-29T18:54:09Z

can you check the other features/settings etc and make sure everything works as expected.

Things seem fine to me so far. I'll work on this branch for a while, see if something fails.

vsvipul · 2019-07-02T21:54:30Z

After running the script to reinstall node-modules, i get this error-

• electron-builder version=20.40.2
  • loaded configuration file=package.json ("build" field)
  • installing production dependencies platform=linux arch=x64 appDir=/home/vipul/Documents/zulip-desktop/app
Error: /usr/bin/node exited with code 1
Output:

> @paulcbetts/spellchecker@4.0.6 install /home/vipul/Documents/zulip-desktop/app/node_modules/@paulcbetts/spellchecker
> node-gyp rebuild

make: Entering directory '/home/vipul/Documents/zulip-desktop/app/node_modules/@paulcbetts/spellchecker/build'
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/base/md5.o
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/base/strings/string_piece.o
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/google/bdict_reader.o

@akashnimare @kanishk98 Any clue about this? A similar issue is here .

vsvipul · 2019-07-02T22:03:18Z

Other than the above problem, Functionality wise, everything seems to be working fine so far. I didn't test the autoupdate. I'll keep testing to see if i can find something.

abhigyank · 2019-07-03T04:39:30Z

Why are we upgrading directly to v5? Didn't we agree to do incremental upgrades for electron?

I think to shift to BrowserView, we do need to upgrade to the latest electron.

vsvipul · 2019-07-03T09:37:30Z

@abhigyank I don't thing we are upgrading because of BrowserView. Maybe, because we want to get to the latest stable version before v3 becomes deprecated.

akashnimare · 2019-07-03T10:36:42Z

After running the script to reinstall node-modules, i get this error-

• electron-builder version=20.40.2
  • loaded configuration file=package.json ("build" field)
  • installing production dependencies platform=linux arch=x64 appDir=/home/vipul/Documents/zulip-desktop/app
Error: /usr/bin/node exited with code 1
Output:

> @paulcbetts/spellchecker@4.0.6 install /home/vipul/Documents/zulip-desktop/app/node_modules/@paulcbetts/spellchecker
> node-gyp rebuild

make: Entering directory '/home/vipul/Documents/zulip-desktop/app/node_modules/@paulcbetts/spellchecker/build'
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/base/md5.o
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/base/strings/string_piece.o
  CXX(target) Release/obj.target/hunspell/vendor/hunspell/src/google/bdict_reader.o

@akashnimare @kanishk98 Any clue about this? A similar issue is here .

Can you remove the node modules and reinstall? I have seen this error but it usually goes away once you reinstall node modules/upgrade node.

I don't thing we are upgrading because of BrowserView. Maybe, because we want to get to the latest stable version before v3 becomes deprecated.

Yeah, our main reason is to get the latest features of v5 and also the browserview.

akashnimare · 2019-07-03T10:37:23Z

We have to go through the release docs from v3 to v5 and see if they have added/removed any apis etc.

vsvipul · 2019-07-03T10:38:29Z

@akashnimare BrowserVIew is available for v3.1 as well, https://electronjs.org/docs/api/browser-view/history

akashnimare · 2019-07-03T20:43:43Z

Electron v5 support the Numpad keys (electron/electron#15689) so I think we should remove the code which was added in 6e76097 (after testing the feature)

akashnimare · 2019-07-04T11:31:17Z

Tested the functionality on macOS, everything works as expected. Currently testing the auto-update and installers. @kanishk98, @vsvipul can you guys make sure we don't miss anything while upgrading to v5?

kanishk98 · 2019-07-04T11:40:23Z

I went through the list of breaking changes and didn't see anything apart from the webviewTag that should concern us. I'll just send you the link to a Paper that contains some notes about possibly useful API changes too.

vsvipul · 2019-07-04T12:07:11Z

@kanishk98 Can you change the comment on this line to -

zulip-desktop/app/renderer/js/preload.js

Line 91 in 1fe62e5

    
           // Zoom from numpad keys is not supported by electron, so adding it through listeners.

"// Electron does not support multiple accelarators for same menu item
// so adding zoom option from numpad keys through listeners here"

kanishk98 · 2019-07-04T12:11:32Z

Yeah, I just noticed this issue. Thanks for the suggestion!

* Addresses breaking change where webviewTag must be set to true when setting webPreferences for a BrowserWindow.

Even though Electron v5 supports numpad keys, we can't add another accelerator to the same menu item.

develar · 2019-07-05T11:38:11Z

It is important for Linux (deb, snap, rpm, AppImage) to upgrade electron-builder to 21.x if you use Electron 5.

"electron-builder": "20.40.2",

<= 20 versions doesn't support Electron 5 correctly.

Also be aware about electron-userland/electron-builder#4005

akashnimare · 2019-07-05T11:42:56Z

@develar thanks for the heads up. @kanishk98 can you update the electron-builder and also electron-updater? I hope that doesn't break the auto-update.

develar · 2019-07-05T11:46:35Z

I hope that doesn't break the auto-update.

Unsafe (not really, but still) changes were done as part of electron-userland/electron-builder#3953 I didn't yet fully manually test new updater, but I don't expect regressions.

kanishk98 · 2019-07-05T12:43:55Z

<= 20 versions doesn't support Electron 5 correctly.

@develar correct me if I'm wrong, but isn't the latest version of electron-builder 20.44.4? Doesn't that mean an update isn't required?

develar · 2019-07-05T12:56:15Z

@kanishk98 Explained here.

kanishk98 · 2019-07-05T20:03:10Z

@develar in the issue you linked (thanks for that, btw) it says that electron versions >= 4.11.0 should use builder v21. Just to confirm, we don't need to update builder and updater if we stick to Electron v4.10.0, right?

develar · 2019-07-05T20:31:03Z

@kanishk98 Please note that I mean not electron, but electron-updater (electron-updater >= 4.11.0 is recommended to use with electron-builder 21.). There are no any mirror changes — electron-updater 4.11.0+ works with 20.x electron-builder, but it is always better to use recommended combination of electron-builder and electron-updater because there are several common dependencies between.

Please note that in electron-updater recently was fixed bug, that probably also affects zulip — electron-userland/electron-builder#3564
For example, wayland for snap is supported only in electron-builder 21.x

So, I strongly recommend to update to electron-builder 21.

kanishk98 · 2019-07-06T05:33:07Z

Alright, sorry for the confusion. I was thrown off by this comment.

So just to be doubly sure, we should not update Electron to v5 without updating builder and updater modules to their latest versions as well, right?

If that's the case, I think we could use some docs about the same, to indicate more clearly which versions of which modules work well with each other. I'd be happy to help out, if required.

develar · 2019-07-06T07:23:34Z

Well, it should be clear that if you want recent version of Electron, then recent version of builder should be used. But I will add note about it to release notes of 21.

In any case soon or later you will be forced to migrate to electron-builder 21 because of electron-userland/electron-builder#3870 :)

vsvipul · 2019-07-18T13:19:59Z

@develar I see that version 21.1 of builder has been released a few hours ago. Will it be good to update to it and simultaneously update our electron version to v5.0.6 ? Also, can you tell the exact updater version we need to update to so that auto-update doesn't break. Upgrading electron is our priority so we need to be sure that something doesn't go wrong.

akashnimare · 2019-07-24T16:34:09Z

@kanishk98 closing this in favour of #793. @vsvipul FYI, feel free to cherry-pick the comments.

zulipbot added the size: XS label Jun 28, 2019

vsvipul approved these changes Jul 4, 2019

View reviewed changes

zulipbot added size: S and removed size: XS labels Jul 4, 2019

kanishk98 added 3 commits July 4, 2019 17:50

deps: Upgrade Electron version to 5.0.6.

98795a0

webview: Enable webviewTag in webPreferences.

24861fd

* Addresses breaking change where webviewTag must be set to true when setting webPreferences for a BrowserWindow.

preload: Update reason for using dom listeners.

c741ed4

Even though Electron v5 supports numpad keys, we can't add another accelerator to the same menu item.

kanishk98 force-pushed the electron-upgrade branch from 315ea4a to c741ed4 Compare July 4, 2019 12:21

akashnimare closed this Jul 24, 2019

kanishk98 mentioned this pull request Dec 9, 2019

Urgent electron upgrade #850

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Upgrade Electron version to 5.0.6 #781

[WIP] Upgrade Electron version to 5.0.6 #781

kanishk98 commented Jun 28, 2019

vsvipul commented Jun 28, 2019

kanishk98 commented Jun 28, 2019

akashnimare commented Jun 29, 2019

kanishk98 commented Jun 29, 2019

vsvipul commented Jul 2, 2019 •

edited

vsvipul commented Jul 2, 2019

abhigyank commented Jul 3, 2019

vsvipul commented Jul 3, 2019

akashnimare commented Jul 3, 2019

akashnimare commented Jul 3, 2019

vsvipul commented Jul 3, 2019

akashnimare commented Jul 3, 2019

akashnimare commented Jul 4, 2019

kanishk98 commented Jul 4, 2019

vsvipul commented Jul 4, 2019 •

edited

kanishk98 commented Jul 4, 2019

develar commented Jul 5, 2019 •

edited

akashnimare commented Jul 5, 2019

develar commented Jul 5, 2019 •

edited

kanishk98 commented Jul 5, 2019

develar commented Jul 5, 2019

kanishk98 commented Jul 5, 2019

develar commented Jul 5, 2019 •

edited

kanishk98 commented Jul 6, 2019

develar commented Jul 6, 2019 •

edited

vsvipul commented Jul 18, 2019

akashnimare commented Jul 24, 2019

[WIP] Upgrade Electron version to 5.0.6 #781

[WIP] Upgrade Electron version to 5.0.6 #781

Conversation

kanishk98 commented Jun 28, 2019

vsvipul commented Jun 28, 2019

kanishk98 commented Jun 28, 2019

akashnimare commented Jun 29, 2019

kanishk98 commented Jun 29, 2019

vsvipul commented Jul 2, 2019 • edited

vsvipul commented Jul 2, 2019

abhigyank commented Jul 3, 2019

vsvipul commented Jul 3, 2019

akashnimare commented Jul 3, 2019

akashnimare commented Jul 3, 2019

vsvipul commented Jul 3, 2019

akashnimare commented Jul 3, 2019

akashnimare commented Jul 4, 2019

kanishk98 commented Jul 4, 2019

vsvipul commented Jul 4, 2019 • edited

kanishk98 commented Jul 4, 2019

develar commented Jul 5, 2019 • edited

akashnimare commented Jul 5, 2019

develar commented Jul 5, 2019 • edited

kanishk98 commented Jul 5, 2019

develar commented Jul 5, 2019

kanishk98 commented Jul 5, 2019

develar commented Jul 5, 2019 • edited

kanishk98 commented Jul 6, 2019

develar commented Jul 6, 2019 • edited

vsvipul commented Jul 18, 2019

akashnimare commented Jul 24, 2019

vsvipul commented Jul 2, 2019 •

edited

vsvipul commented Jul 4, 2019 •

edited

develar commented Jul 5, 2019 •

edited

develar commented Jul 5, 2019 •

edited

develar commented Jul 5, 2019 •

edited

develar commented Jul 6, 2019 •

edited