Seamless-Updates - added flow for automatic updates for releases #10857
Conversation
AliceHincu
changed the title
AliceHincu
changed the title
| @@ -462,6 +474,14 @@ export default class ElectronAppWrapper { | |||
| this.customProtocolHandler_ ??= handleCustomProtocols(logger); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| public initializeAutoUpdaterService(logger: LoggerWrapper, initializedShim: any, devMode: boolean, allowPrereleaseUpdates: boolean) { | |||
| if (!shim.isLinux()) { | |||
There was a problem hiding this comment.
Here please only list the platforms that are supported, which would be isMacOS and isWindows. Joplin works also on BeOS and maybe others, so it's better to explicitly list what we support instead what we don't support
| @@ -462,6 +474,14 @@ export default class ElectronAppWrapper { | |||
| this.customProtocolHandler_ ??= handleCustomProtocols(logger); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| public initializeAutoUpdaterService(logger: LoggerWrapper, initializedShim: any, devMode: boolean, allowPrereleaseUpdates: boolean) { | |||
There was a problem hiding this comment.
Small thing, but could you please use the same name as the setting variable for the prerelease boolean. So includePreReleases
| public constructor() { | ||
| private enableDevMode = true; // force the updater to work in "dev" mode | ||
| private enableAutoDownload = false; // automatically download an update when it is found | ||
| private allowPrerelease_ = false; |
There was a problem hiding this comment.
Here as well - includePreReleases. Unless there's a good reason we should try to use consistent naming everywhere, it makes it easier to search for code afterwards
packages/app-desktop/services/autoUpdater/AutoUpdaterService.ts
Outdated
Show resolved
Hide resolved
packages/app-desktop/app.ts
Outdated
| // Note: Auto-update is a misnomer in the code. | ||
| // The code below only checks, if a new version is available. | ||
| // We only allow Windows and macOS users to automatically check for updates | ||
| if (shim.isWindows() || shim.isMac()) { | ||
| const runAutoUpdateCheck = () => { | ||
| if (Setting.value('autoUpdateEnabled')) { | ||
| void checkForUpdates(true, bridge().window(), { includePreReleases: Setting.value('autoUpdate.includePreReleases') }); | ||
| } | ||
| }; | ||
|
|
||
| // Initial check on startup | ||
| shim.setTimeout(() => { runAutoUpdateCheck(); }, 5000); | ||
| // Then every x hours | ||
| shim.setInterval(() => { runAutoUpdateCheck(); }, 12 * 60 * 60 * 1000); | ||
| } | ||
|
|
There was a problem hiding this comment.
Here we have still the same problem - we actually want to keep this code, and it should be enabled by default. The new AutoUpdaterService should be disabled by default.
What I meant with the feature flag is that you should create a new setting, for example featureFlag.autoUpdaterServiceEnabled, which defaults to false. And you use this to enabled/disable the old code and disable/enable the new code.
Have a look in Setting.ts - there are some examples of commented out feature flags (they start with featureFlag.). Create a similar setting value and use it here.
We need this for safety - because if there's any issue with your change, a lot of people won't be notified any more when a new version is available. So by default everybody use the old method, and the user can explicitly enable the new method (like I will do for example)
This PR contains:
enableAutoDownload = false. This will be set to true in another PR. I will check how VsCode handles downloads (whether it automatically downloads them or after the user agrees to update).The reason why I gave the variable the name "initializedShim": inside the logic of fetching prereleases (which will be in the next PR), I can't use the boolean functions that check the platform type (ex:
isWindows()), but I can usesetInterval()andsetTimeout()functions. If I also import it in the file under a different name, I can access the platform check functions, but not thesetIntervalandsetTimeoutfunctions. I'll try to see why this happens in the next PR.Also, even if all the review changes are implemented, please do not merge this until I fully test on platforms different from Windows.