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

checkForUpdates() with dev-app-update.yml returns null result instead of expected object #6863

Closed
Nantris opened this issue May 13, 2022 · 8 comments · Fixed by #7117
Closed

Comments

@Nantris
Copy link

Nantris commented May 13, 2022

  • Electron-Builder Version: 22.14.13
  • Node Version: 14.17.6
  • Electron Version: 17.4.0
  • Electron Type (current, beta, nightly): current
  • Target: Unpackaged/DEV (but running on Windows)

We have a code like below and I've confirmed the path is correct and that the dev-app-update.yml points to our S3 bucket, but the updateCheckResult is always null. Shouldn't we be receiving the proper versionInfo and updateInfo in an object?

I've already confirmed that the updater works outside of DEV, so why null result in DEV? This did not used to be the behavior but I don't know when it changed. We haven't touched our code or changed our update bucket path in ages.

autoUpdater.updateConfigPath = path.join(
    __dirname,
    'dev-app-update.yml'
);

autoUpdater
    .checkForUpdates()
    .then(updateCheckResult => {
        handleUpdateCheckResult(updateCheckResult);
    })
@Nantris
Copy link
Author

Nantris commented May 13, 2022

Additionally I see this in my logs, despite never actually calling checkForUpdatesAndNotify. It's also not a firewall issue.

Skip checkForUpdatesAndNotify because application is not packed
Updater can connect: false

@DogFoxX
Copy link

DogFoxX commented May 24, 2022

I also get Skip checkForUpdatesAndNotify because application is not packed although I'm calling autoUpdater.checkForUpdates(). I have dev-app-update.yml in root, and running

function isDev() {
   return !app.isPackaged;
};

if (isDev()) {
   autoUpdater.checkForUpdates();
};

doesn't change a thing.

Currently on v5.0.1, didn't have this issue previously on v4.6.5. I really don't want to package my app every time just to test updates.

@Nantris
Copy link
Author

Nantris commented May 26, 2022

Looks like I tested originally with version 5.0.0. It's still an issue if we manually upgrade to the latest as well (5.0.4.)

@develar any thoughts on this?

@mmaietta
Copy link
Collaborator

Yes, I see that behavior specified here:

checkForUpdates(): Promise<UpdateCheckResult | null> {
if (!this.isUpdaterActive()) {
return Promise.resolve(null)
}

I wasn't aware that previously it could still be used locally without packaging the app. The isUpdaterActive check was moved into checkForUpdates due to users reporting error logs that it was checking for a non-existent update file. This was most notable for any distributable that doesn't support auto-update

Not sure what we should do. Seems like the warning log message needs to be updated, and it seems reasonable that we allow dev-app-update.yml to be usable. Can we use a different flag, or add an arg to checkForUpdates(forceDev = false) { ... } with default false?

@Nantris
Copy link
Author

Nantris commented Jun 29, 2022

@mmaietta thanks very much for digging in to find the source of the issue. Have you had a chance to consider possible resolutions any further?

@Nantris
Copy link
Author

Nantris commented Aug 31, 2022

Any update on this?

@mmaietta
Copy link
Collaborator

mmaietta commented Sep 1, 2022

My bad, I honestly forgot about this one.
I see this code is already checking isPackaged

get appUpdateConfigPath(): string {
return this.isPackaged ? path.join(process.resourcesPath!, "app-update.yml") : path.join(this.app.getAppPath(), "dev-app-update.yml")
}

I'm wondering if it's better to add a property to the Updater such as forceDevUpdateConfig = false to allow bypassing the isUpdaterActive check

  public isUpdaterActive(): boolean {
    if (!this.app.isPackaged && !this.forceDevUpdateConfig) {
      this._logger.info("Skip checkForUpdates because application is not packed and dev update config check explicitly disabled")
      return false
    }
    return true
  }

Otherwise, both checkForUpdatesAndNotify and checkForUpdates have to manually pass in a new var, which doesn't seem optimized to me.

What are your thoughts

@Nantris
Copy link
Author

Nantris commented Sep 1, 2022

Thanks very much for the update @mmaietta!

I'm maybe not the best judge of solutions since I'm not all that familiar with the rest of the code, but I think what you've laid out makes sense.

My only thought is, would it possibly make more sense to have something like this.updaterEnabled = this.app.isPackaged || this.forceDevUpdateConfig`

Again though, I'm rather oblivious to the code outside of what you've shared.

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