Skip to content

Conversation

@rhy3h
Copy link
Contributor

@rhy3h rhy3h commented Feb 25, 2025

Implemented the issue #164

  • autoCheck - which defaults to true. It can be set to false to disable automatic update checks.
  • checkForUpdates() - manually check for updates.

However, I did not implement the final installUpdate function
Because the correct installation process should be executed after the file has been downloaded

autoUpdater.once('update-downloaded', () => {
  autoUpdater.quitAndInstall();
});

@rhy3h rhy3h requested a review from a team as a code owner February 25, 2025 06:17
@rhy3h
Copy link
Contributor Author

rhy3h commented Mar 15, 2025

Friendly ping! Any feedback on this?

@erickzhao
Copy link
Member

@rhy3h Hey! Haven't gotten around to reviewing it yet, thanks for the ping. Hopefully can get that done for you in the next week or two?

@erickzhao erickzhao assigned erickzhao and unassigned erickzhao Mar 15, 2025
@rhy3h
Copy link
Contributor Author

rhy3h commented Mar 16, 2025

@erickzhao Hey! No worries at all, I really appreciate you taking the time. I just started contributing to PRs recently, so I’d love to know if I’m doing things correctly. No rush at all!

@rhy3h
Copy link
Contributor Author

rhy3h commented Apr 8, 2025

Just checking in again—no rush at all, and really appreciate your time. Would love any feedback whenever you get a chance!

@dariuszjastrzebski
Copy link

@erickzhao have you had chance to check this PR? This changes gain greater control over when and how updates are managed in the application. I would be grateful for your feedback

@rhy3h
Copy link
Contributor Author

rhy3h commented May 13, 2025

FYI
I just find out linux do not support auto update
so I skip the test when in linux platform

Comment on lines +98 to +101
/**
* @param {Boolean} autoCheck Decides whether to automatically check for updates
* Defaults to `true`.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @param {Boolean} autoCheck Decides whether to automatically check for updates
* Defaults to `true`.
*/
/**
* @param {Boolean} autoCheck Decides whether to automatically check for updates
* @default true
*/

Copy link
Contributor Author

@rhy3h rhy3h May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other comments

/**
* @param {Object} logger A custom logger object that defines a `log` function.
* Defaults to `console`. See electron-log, a module
* that aggregates logs from main and renderer processes into a single file.
*/
readonly logger?: L;
/**
* @param {Boolean} notifyUser Defaults to `true`. When enabled the user will be
* prompted to apply the update immediately after download.
*/

those writing style uses indentation
so I do that too
which one should I follow

// check for updates right away and keep checking later
autoUpdater.checkForUpdates();
setInterval(() => {
if (autoCheck) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (autoCheck) {
if (autoCheck === true) {

I wonder if we want to be stricter about actually passing true rather than something truthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (opts.notifyUser) {

I follow this, so I didn't do strict equal
I agree with you, it might need stricter

src/index.ts Outdated
}
}

export function checkForUpdates() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a JSDoc comment for the new manual checkForUpdates function as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rhy3h@7019793
please see this commit

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking comment on IMO bad api surface

@rhy3h rhy3h requested a review from MarshallOfSound May 21, 2025 12:11
@rhy3h
Copy link
Contributor Author

rhy3h commented Jul 1, 2025

Friendly ping!

@rhy3h rhy3h requested a review from erickzhao July 7, 2025 07:27
@SoulKa
Copy link

SoulKa commented Nov 3, 2025

How about just setting the updateInterval <= 0 to disable auto updates? This would omit the need for a new property. @MarshallOfSound is that what you meant with bad API?

We've just introduced your auto updater module in our application and I would very much welcome more control over the update process as suggested in this PR since we're trying to be gentle on the internet usage. Our current solution is just to set a very high interval, but that's a hack in my opinion.

@peppescg
Copy link

peppescg commented Nov 4, 2025

Hey guys, I am really interesting to this PR, cause I would like to stop the checkForUpdates as well ( my 2c cause updateInterval is expecting a string || undefined we can use empty string in order to skip the checks)


I have this weird behavior, looking to the logs I saw an issue where the update (triggered every 10m as default), is available but during the downloading I receive the update-not-available event.

[info]  [update] checking for updates...
[info]  update-available; downloading...
[info]  [update] update is available, starting download...
[info]  update-not-available

I am handling notifyUser: false use case, showing the modal and performing manually the dialog and the autoUpdater.quitAndInstall().

Do you have an idea about this use case?

@rhy3h
Copy link
Contributor Author

rhy3h commented Nov 7, 2025

Hi, just wondering if there’s anything else I should do to get this PR ready for merge.
Please let me know if any changes or updates are needed.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants