-
Notifications
You must be signed in to change notification settings - Fork 13
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
updater: fix update is only available after download #1345
Conversation
@@ -54,7 +54,7 @@ module.exports = async function setupUpdater ( | |||
/** @type {import('./typings').Context} */ ctx | |||
) { | |||
ctx.getUpdaterStatus = function getUpdaterStatus () { | |||
return { updateAvailable } | |||
return { updateAvailable: updateDownloaded } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to rename the places reading this property too. Otherwise, we are using different names ("update available" vs "update downloaded") for the same thing and this could create confusion in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this too, but decided against this because I think the places outside this library shouldn't have to care about the finer details of updating. I could see however how updateDownloaded
is more clear. I was just thinking that as an outsider, "update downloaded" could be more confusing than "update available" (ok it's downloaded, are there any other steps?). Maybe instead something like readyToUpdate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your reasoning. 👍🏻
I like the idea of renaming the property to readyToUpdate
or perhaps updateReady
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, that's a really involved change. It would be better to land the last commit in a standalone PR. Otherwise, it's not clear which changes are related to refactor-rename and which changes are modifying the behaviour.
I skimmed through the changes and didn't see any obvious issues. I did not test the new version manually, though.
This reverts commit bf910b3.
No description provided.