Page MenuHomePhabricator

[desktop] Add auto updating
ClosedPublic

Authored by michal on Jan 20 2023, 4:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 12:41 AM
Unknown Object (File)
Fri, Nov 8, 2:54 AM
Unknown Object (File)
Fri, Nov 8, 2:53 AM
Unknown Object (File)
Fri, Nov 8, 2:53 AM
Unknown Object (File)
Fri, Nov 8, 2:53 AM
Unknown Object (File)
Fri, Nov 8, 2:53 AM
Unknown Object (File)
Fri, Nov 8, 2:53 AM
Unknown Object (File)
Mon, Nov 4, 6:17 PM
Subscribers

Details

Summary

Uses the autoUpdater module and check for updates every 5 minutes. Exposes to the web app:

  • current version
  • listener for a new version
  • method to trigger the update.

Depends on D6321

Test Plan

Tested with the later diffs that introduce UI in the web app. Created a few version of the app and checked if the download and automatic update is triggered correctly after uploading a new one (on both macos and windows).

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

desktop/src/main.js
248–254 ↗(On Diff #21140)

We want to run auto update only if the app is packaged and not running in dev mode. Additionally initAutoUpdate() will throw an error on macOS if the app isn't signed to I've added the try catch.

desktop/src/preload.js
16 ↗(On Diff #21140)

This is sent synchronously and not asynchronously because we want the desktop app version to be available to the web app from the start. This runs only once, at the start, and the corresponding function in main.js is minimal so blocking the renderer thread shouldn't be a problem.

tomek requested changes to this revision.Jan 26 2023, 3:58 AM
tomek added inline comments.
desktop/src/auto-update.js
12 ↗(On Diff #21140)

Was it decided somewhere that 5 min interval is a good idea? What are the good practices about it?

13 ↗(On Diff #21140)

From the docs

Note: If an update is available it will be downloaded automatically. Calling autoUpdater.checkForUpdates() twice will download the update two times.

So if an update takes longer than 5 mins, we will end up downloading it multiple times. We should probably stop asking about the updates after update-available is emitted.

This revision now requires changes to proceed.Jan 26 2023, 3:58 AM

Change auto updating logic.

desktop/src/auto-update.js
12 ↗(On Diff #21140)

I've changed it to 10min. It's the default for update-electron-app package which is the official package for easier setup of auto updating (we don't use it because we use hazel update server and we want in-app "update available" modals).

Not sure what the correct time should be, I don't think it was discussed anywhere.

13 ↗(On Diff #21140)

I've made it so the update check is scheduled after the update-not-available event and update-downloaded. I find it clearer than removing the interval in update-available and then recreating it in update-downloaded.

tomek added inline comments.
desktop/src/auto-update.js
12 ↗(On Diff #21606)

Let's add a unit to the name of the const

26 ↗(On Diff #21606)

We should be able to simplify the syntax

13 ↗(On Diff #21140)

Great, that makes sense!

This revision is now accepted and ready to land.Jan 30 2023, 8:52 AM

Changed the name to reflect that it's ms. We can't simplify the calls because these methods need to be binded to the autoUpdater object.