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

electron-updater: feature request - custom minimumSystemVersion validation #8682

Open
kavsingh opened this issue Nov 13, 2024 · 10 comments · May be fixed by #8692
Open

electron-updater: feature request - custom minimumSystemVersion validation #8682

kavsingh opened this issue Nov 13, 2024 · 10 comments · May be fixed by #8692

Comments

@kavsingh
Copy link

kavsingh commented Nov 13, 2024

  • Electron-Builder Version: 24.13.3 (want to update to 25.x, blocked by this issue)
  • Electron-Updater Version: 5.3.0 (want to update to 6.x, blocked by this issue)
  • Node Version: 20.16.0
  • Electron Version: 31.4.0
  • Electron Type (current, beta, nightly): current
  • Target: darwin x64, darwin arm64, win32 x64

Before minimumSystemVersion was read in-app by electron-updater - #8108 - we rolled our own minimumSystemVersion checks since we had a pressing need.

When building our application, we write minimumSystemVersion to the update yaml as the "human readable" product version on macOS, e.g 12.0.0 for Monterey (to match mac.minimumSystemVersion in our electron builder config). The changes in #8108 force minimumSystemVersion to be checked against against os.release. While os.release is a sensible default, unfortunately we cannot switch up to use it in our publish yml files without major disruptions to our user base - app installs already in the wild can end up blocked from updating or erroneously updated.

It would be great to have an option to use a custom validator for minimumSystemVersion, or to disable it and handle minimumSystemVersion ourselves.

Am happy to open a PR for this.

@mmaietta
Copy link
Collaborator

Thoughts on a metadata field in the app-update.yml that is of type [key: string]: any? Then during update-available, you can check the update for that information in the UpdateInfo object

We'd probably want to set a file size limit (arbitrarily?) on the app-update.yml file though to prevent abuse of the metadata field that would increase file size and the latency (slow download) of detecting an update is available

@kavsingh
Copy link
Author

kavsingh commented Nov 13, 2024

hi @mmaietta! it's a good suggestion but unfortunately that would only work for a new release of our app onwards. the app has been in general release for quite some time and it wouldn't work for users who already have current and previous versions of the app installed, unless i'm missing something:

already released versions in the wild will continue to look for minimumSystemVersion in update info, expecting a version in line with macOS's product version, and not os.release. they wouldn't be looking for the metadata field.

if we change up our writing of minimumSystemVersion when publishing our app to os.release to match what electron-updater expects: already installed versions would not be able to update. say a user is on macOS 13, and we change minimumSystemVersion published in our update info from 12 as we have it to 21 (an os.release version). those installed apps would then always fail to register an available update.

if we leave our writing of minimumSystemVersion in update info as-is, and adopt the latest electron-updater without modifications: already installed versions would not be affected, but we lose the ability to deprecate support for macOS versions via auto update going forward. again taking a user on macOS 13, and we want to change our minimumSystemVersion to 14 and write it as such. a user using the app with latest electron-updater on macOS 13 would end up getting the update, since os.release would resolve to something like 21, and the check would pass. the user then ends up updating to a version of the app they cannot use.

the added catch is that to get existing installations in line with consuming new metadata, we need them to be able to auto update, which is the core issue 😅

@mmaietta
Copy link
Collaborator

mmaietta commented Nov 13, 2024

If I'm understanding correctly, you want custom minimum system version validation on already-installed versions of your application? If so, that's not possible with electron-updater. Feature requests like this will only apply to new deployments, the currently installed applications use the electron-updater version it was bundled with. The only way out of this situation is to revert your non-backward-compatible changes (or create a separate release branch?) so you can update electron-updater (assuming we proceed with metadata or some other approach), from which then you'd be able to move forward with your non-compatible changes.

My apologies in advance if I missread/interpreted what you're saying

@kavsingh
Copy link
Author

kavsingh commented Nov 13, 2024

hey @mmaietta

thanks so much for keeping up with this!

you want custom minimum system version validation on already-installed versions of your application?

ah no not quite, as you said that would clearly not be possible. we would like to able to customise how electron updater checks minimumSystemVersion here for new releases of our app should we update to the new version of electron-updater

this would let us keep writing the version in our publisher yml as e.g. 12 for macOS Monterey and not the os.release version for compatiblity with already released app versions, while allowing new versions to check minimumSystemVersion against eg sw_vers instead of os.release

let me know if that makes more sense

@mmaietta
Copy link
Collaborator

Hmmm sw_vers is MacOS-specific, so we'd need similar setups for linux/win or just have a specific flow for mac which seems messy/hard to track bugs with.

Right now minimumSystemVersion isn't able to be set in the electron-builder config for each OS, which I believe has been a previous request. If I implemented a way to have minimumSystemVersion set for each distributable (i.e. platform-specific mac/nsis/etc.) within electron-builder.config.js, would that resolve your issue instead? Then you don't need to custom set any version in your update yaml.

Regardless, it looks like we'll still run into the field conflicting due to var name. In order to have a platform-specific minimumSystemVersion without overlapping with the already-human-readable mac.minimumSystemVersion, the var will need to be renamed. Now thinking about it further though, we can't rename the var in the UpdateInfo without breaking electron-updater compatibility...so I'm feeling hosed on renaming any vars even with electron-builder in a major-semver alpha release state.

An alternative route would require one migration step in between for your users. What I envision:

  • Add an additional field to your app-update.yml that mirrors minimumSystemVersion field -> release an update.
  • Wait for most users to update
  • Update electron-updater to latest version

I think that'd work?

@kavsingh
Copy link
Author

kavsingh commented Nov 13, 2024

thanks again for all the discussion!

Hmmm sw_vers is MacOS-specific, so we'd need similar setups for linux/win or just have a specific flow for mac which seems messy/hard to track bugs with.

minimumSystemVersion without overlapping with the already-human-readable mac.minimumSystemVersion, the var will need to be renamed. Now thinking about it further though, we can't rename the var in the UpdateInfo without breaking electron-updater compatibility..

totally agree that renaming the variable in electron updater would not be the way to go, and that something like a sw_vers is way too specific to work into electron-updater itself

i was thinking more along the lines of being able to assign a version check function in-app, so that a consumer of the library can do something like:

import { autoUpdater } from "electron-updater";

autoUpdater.validateMinimumSystemVersion = (logger, minimumSystemVersion?: string): boolean => {
  // ... some custom version check that returns false if invalid
}

used in AppUpdater#isUpdateAvailable

if (this.validateMinimumSystemVersion) {
  if (this.validateMinimumSystemVersion(this._logger, updateInfo.minimumSystemVersion) === false) {
    return false;
  }
} else {
  // ... perform current default system version check against os.release
}

essentially the end user can decide if the version is valid for their setup. but i can understand if that might add maintenance overhead, not to mention requests for extending that for other attributes down the line

the intermediary release plan you describe is something we've thought about as well - we can give that more thought on our end, though it might take a while for sufficient uptake. in that case, is it advisable to upgrade electron-builder to v25.x while keeping updater at v5.x? we'd like to keep builder at least up-to-date.

we could also look at making use of the update-not-available event to work around our issue, but that would rely on

  • having full update info available in the event (which i think is the case)
  • still being able to download and install the update if we decide the update is in fact valid in response to the event

@mmaietta
Copy link
Collaborator

Hmmm, I like where you're going with this! Perhaps we can expand the functionality to this.isUpdateSupported(this._logger, updateInfo) to be more generic. If we keep it the way you have it, then we have to have the "hook" specifically for minimumSystemVersion so I'm trying to figure out a way around it. Maybe if isUpdateSupported is set, we default to that logic instead of any validation other than staging matcher and downgrade logic

@kavsingh
Copy link
Author

kavsingh commented Nov 14, 2024

i think that's a great idea! i was hesistant to make it too generic to avoid breaking any other validation logic electron-updater relies on, but what you suggest makes a lot of sense

@mmaietta
Copy link
Collaborator

@kavsingh , can you please try out this patch-package? It adds the hook isUpdateSupported to the AppUpdater class that can be overridden by the application for additional verifications of the UpdateInfo.

Let me know how it works out and if it serves your needs. If so, then I'll merge in the PR
electron-updater+6.4.0-alpha.1.patch

diff --git a/node_modules/electron-updater/out/AppUpdater.js b/node_modules/electron-updater/out/AppUpdater.js
index 48e23fb..4616cdb 100644
--- a/node_modules/electron-updater/out/AppUpdater.js
+++ b/node_modules/electron-updater/out/AppUpdater.js
@@ -76,6 +76,14 @@ class AppUpdater extends events_1.EventEmitter {
         this._appUpdateConfigPath = value;
         this.configOnDisk = new lazy_val_1.Lazy(() => this.loadUpdateConfig());
     }
+    get isUpdateSupported() {
+        return this._isUpdateSupported;
+    }
+    set isUpdateSupported(value) {
+        if (value) {
+            this._isUpdateSupported = value;
+        }
+    }
     constructor(options, app) {
         super();
         /**
@@ -148,6 +156,7 @@ class AppUpdater extends events_1.EventEmitter {
          */
         this.signals = new main_1.UpdaterSignal(this);
         this._appUpdateConfigPath = null;
+        this._isUpdateSupported = (updateInfo) => this.checkIfUpdateSupported(updateInfo);
         this.clientPromise = null;
         this.stagingUserIdPromise = new lazy_val_1.Lazy(() => this.getOrCreateStagingUserId());
         // public, allow to read old config for anyone
@@ -307,18 +316,8 @@ class AppUpdater extends events_1.EventEmitter {
         if ((0, semver_1.eq)(latestVersion, currentVersion)) {
             return false;
         }
-        const minimumSystemVersion = updateInfo === null || updateInfo === void 0 ? void 0 : updateInfo.minimumSystemVersion;
-        const currentOSVersion = (0, os_1.release)();
-        if (minimumSystemVersion) {
-            try {
-                if ((0, semver_1.lt)(currentOSVersion, minimumSystemVersion)) {
-                    this._logger.info(`Current OS version ${currentOSVersion} is less than the minimum OS version required ${minimumSystemVersion} for version ${currentOSVersion}`);
-                    return false;
-                }
-            }
-            catch (e) {
-                this._logger.warn(`Failed to compare current OS version(${currentOSVersion}) with minimum OS version(${minimumSystemVersion}): ${(e.message || e).toString()}`);
-            }
+        if (!(await Promise.resolve(this.isUpdateSupported(updateInfo)))) {
+            return false;
         }
         const isStagingMatch = await this.isStagingMatch(updateInfo);
         if (!isStagingMatch) {
@@ -333,6 +332,22 @@ class AppUpdater extends events_1.EventEmitter {
         }
         return this.allowDowngrade && isLatestVersionOlder;
     }
+    checkIfUpdateSupported(updateInfo) {
+        const minimumSystemVersion = updateInfo === null || updateInfo === void 0 ? void 0 : updateInfo.minimumSystemVersion;
+        const currentOSVersion = (0, os_1.release)();
+        if (minimumSystemVersion) {
+            try {
+                if ((0, semver_1.lt)(currentOSVersion, minimumSystemVersion)) {
+                    this._logger.info(`Current OS version ${currentOSVersion} is less than the minimum OS version required ${minimumSystemVersion} for version ${currentOSVersion}`);
+                    return false;
+                }
+            }
+            catch (e) {
+                this._logger.warn(`Failed to compare current OS version(${currentOSVersion}) with minimum OS version(${minimumSystemVersion}): ${(e.message || e).toString()}`);
+            }
+        }
+        return true;
+    }
     async getUpdateInfoAndProvider() {
         await this.app.whenReady();
         if (this.clientPromise == null) {

@kavsingh
Copy link
Author

hi @mmaietta, just tried it out and it works well for our needs! thanks so much 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants