Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Updater module #292
Conversation
sindresorhus
added some commits
Aug 27, 2012
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 4, 2012
Owner
Request for later: once we land this it would be great to document some of what it does / some of whats in this ticket in the wiki for future internal reference. Doesn't need to be at launch time, can be whenever we have time.
Request for later: once we land this it would be great to document some of what it does / some of whats in this ticket in the wiki for future internal reference. Doesn't need to be at launch time, can be whenever we have time. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Sure thing |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 4, 2012
Owner
- Performed a code review. So far everything looks solid \o/ (please see comments below)
- Testing
Notes on user experience/possibly bugs:
- Once the updater has completed running, what's the expected behavior? If I'm a completely new user and I saw http://cl.ly/image/1047151V0g0o. I wouldn't know whether I should be running
init
again, restarting the terminal or doing something else. Is this the debug experience or are users expected to run their last command again? Should we automatically try running it for them once the update check is complete? - When I fired up
init
it was clear that yeoman was updating, but I wasn't sure what to expect or how soon I might be able to actually use it. Perhaps we should add a line at the start saying 'Good day, sir or madam. I'm going to run an update check real quick. Shouldn't take a minute!'. (or something) - I wonder if we need to be doing more stringent x.y.z testing on version numbers. Please correct me if I've interpreted this stupidly: http://cl.ly/image/0h2l360S3o05 thinks we need a patch update, I think incorrectly. I updated the version number to 0.5.13, which is higher than the current 0.3.14. It appears the updater checked against the last number only (z), not taking into account the others (x and y). In this case it would still update, which is totally fine, but the expected behavior of how often/how it prompts might not be what we want.
- I might be reading this wrong, but to me (and again, I'm probably being silly) I felt a bit confused reading through:
You have version 0.5.13
Latest version is 0.3.14
Update checks complete
Update available: 0.5.13 (current: 0.3.14)
Updating grunt
It says that I have version 0.5.13 and that 0.3.14 is the latest version. Great. In the update available line however it says current 0.3.14. Does this mean the update available is 0.3.14? If so, why show the 0.5.13 in this context? Would that not confuse the user? I read this as 0.5.13 was available, but 0.3.14 was the update available (which feels weird to say). I'd just show the version available remotely personally.
Please feel free to let me know if I misinterpreted anything. Sure I have somewhere :)
Great work on this otherwise. The refactor looks awesome!
Notes on user experience/possibly bugs:
It says that I have version 0.5.13 and that 0.3.14 is the latest version. Great. In the update available line however it says current 0.3.14. Does this mean the update available is 0.3.14? If so, why show the 0.5.13 in this context? Would that not confuse the user? I read this as 0.5.13 was available, but 0.3.14 was the update available (which feels weird to say). I'd just show the version available remotely personally. Please feel free to let me know if I misinterpreted anything. Sure I have somewhere :) Great work on this otherwise. The refactor looks awesome! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
sindresorhus
Sep 4, 2012
Owner
Once the updater has completed running, what's the expected behavior? If I'm a completely new user and I saw http://cl.ly/image/1047151V0g0o. I wouldn't know whether I should be running init again, restarting the terminal or doing something else. Is this the debug experience or are users expected to run their last command again? Should we automatically try running it for them once the update check is complete?
Right now it just exits, but I like the idea of capturing the command and just continuing.
When I fired up init it was clear that yeoman was updating, but I wasn't sure what to expect or how soon I might be able to actually use it. Perhaps we should add a line at the start saying 'Good day, sir or madam. I'm going to run an update check real quick. Shouldn't take a minute!'. (or something)
I'm not sure that's necessary. The update check is superquick and the update process itself is heavily console.log'd and process.pipe'd. Anyone else feel this is necessary? The reason I'm a bit against it is because I would like to keep the updater module fairly generic.
I wonder if we need to be doing more stringent x.y.z testing on version numbers. Please correct me if I've interpreted this stupidly: http://cl.ly/image/0h2l360S3o05 thinks we need a patch update, I think incorrectly. I updated the version number to 0.5.13, which is higher than the current 0.3.14. It appears the updater checked against the last number only (z), not taking into account the others (x and y). In this case it would still update, which is totally fine, but the expected behavior of how often/how it prompts might not be what we want.
That is because you've set the current version to higher than the latest version, which would never happen in a real scenario. Actually the original updater did check the last number first, but that was wrong, since it would return 'patch' on current 0.2.0 and latest 0.3.1, which obviously is 'minor'.
Right now it just exits, but I like the idea of capturing the command and just continuing.
I'm not sure that's necessary. The update check is superquick and the update process itself is heavily console.log'd and process.pipe'd. Anyone else feel this is necessary? The reason I'm a bit against it is because I would like to keep the updater module fairly generic.
That is because you've set the current version to higher than the latest version, which would never happen in a real scenario. Actually the original updater did check the last number first, but that was wrong, since it would return 'patch' on current 0.2.0 and latest 0.3.1, which obviously is 'minor'. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 4, 2012
Owner
Right now it just exits, but I like the idea of capturing the command and just continuing.
is this something you think you'll have time to implement today? :)
is this something you think you'll have time to implement today? :) |
sindresorhus
added some commits
Sep 4, 2012
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
sindresorhus
Sep 4, 2012
Owner
@addyosmani Done :)
I just put everything in bin/yeoman into a init() method and execute that after the update check. I had to change some of the semantics. Previously the getUpdate() callback was executed after the update check. It's now executed after the update is complete, which makes more sense, now that the updater handles all the update stuff and console.logging anyway. Thoughts?
We need to look into how to better handle the various error points at a later time.
@addyosmani Done :) I just put everything in bin/yeoman into a init() method and execute that after the update check. I had to change some of the semantics. Previously the getUpdate() callback was executed after the update check. It's now executed after the update is complete, which makes more sense, now that the updater handles all the update stuff and console.logging anyway. Thoughts? We need to look into how to better handle the various error points at a later time. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 5, 2012
Owner
Excellent. Pending some minor further testing we can land this in the morning. Thanks!
Excellent. Pending some minor further testing we can land this in the morning. Thanks! |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
sindresorhus
Sep 5, 2012
Owner
@mklabs If you have a chance. Would love your comments on it.
Let me know if you need a look through or testing on the config stuff ;)
@mklabs If you have a chance. Would love your comments on it. Let me know if you need a look through or testing on the config stuff ;) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 5, 2012
Owner
Testing so far it seems to work fine. @sindresorhus Would you like us to hold off on a review from @mklabs before merging?
Testing so far it seems to work fine. @sindresorhus Would you like us to hold off on a review from @mklabs before merging? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Yes |
added a commit
that referenced
this pull request
Sep 5, 2012
addyosmani
merged commit c18d469
into
master
Sep 5, 2012
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
addyosmani
Sep 5, 2012
Owner
Just to confirm that @sindresorhus was happy with a merge so its been done.
Just to confirm that @sindresorhus was happy with a merge so its been done. |
sindresorhus commentedSep 3, 2012
Fixes #179
This should be done know, but it needs some thorough testing. @addyosmani @paulirish @mklabs and anyone else that could help out.
It was hard testing without actually having a
yeoman
module in NPM to test against, but I made it work by downgrading one of our dependencies, eggrunt-mocha
and then try it against that one, which seems to work.@mklabs Since you're our code master, would you mind taking a look and let me know if there's anything weird going on?
Testing tips
patch
type, 0.4.0 to seeminor
and 1.0.0 tomajor
.Short explanation
The API is simple. You call updater.getUpdate() each time your module is run and it will handle the rest for you. It will skip the update if the time since last update hasn't passed.
updateCheckInterval
andupdatePromptTimeLimit
can be easily overridden if the user want something else.optOut
and the last time the update check was run is saved in~/.config/npm-updater/[packageName].json
. To opt out of auto-update you can just switch thefalse
totrue
.For testing I've set
updateCheckInterval
to 10sec andupdatePromptTimeLimit
to 20sec, so if you we try again before 10sec has passed it will be skipped, then just wait some seconds and try again.The module comes with a prefilter (
updater.shouldUpdate
) that you can override to do your own logic on what to do on different update types. Currently we do this, but can easily be changed by the author: