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

Add automatic check for updates that nags the user when there's a new version available #1429

Merged
merged 8 commits into from Nov 14, 2016

Conversation

sebmck
Copy link
Contributor

@sebmck sebmck commented Oct 25, 2016

No description provided.

@Daniel15
Copy link
Member

Awesome! I'll take a look at this later today or tomorrow :)


async _checkUpdate(): Promise<void> {
let latestVersion = await this.config.requestManager.request({
url: 'https://yarnpkg.com/latest-version',
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to better automate this getting updated

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm going to have a webhook that runs when releases are updated, or just a cronjob. The tricky thing is that we can't bump the version number until we verify that all files are attached to the release, and the Windows installer is attached separately from everything else (as it's built on AppVeyor).


checkUpdate() {
if (!process.stdout.isTTY || isCI) {
// don't show upgrade dialog on CI or non-TTY terminals
Copy link
Member

Choose a reason for hiding this comment

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

I think this is still valuable for non-TTY environments. Why should we avoid showing a notice about Yarn being outdated just because the user is piping the output to a file, for example?

@@ -72,6 +72,10 @@ const messages = {
unexpectedError: 'An unexpected error occurred, please open a bug report with the information provided in $0.',
jsonError: 'Error parsing JSON at $0, $1.',

yarnOutdated: "Your current version of Yarn is out of date. The latest version is $0 while you're on $1.",
yarnOutdatedInstaller: 'To upgrade, download the latest installer at $0.',
yarnOutdatedCommand: 'To upgrade, run $0.',
Copy link
Member

Choose a reason for hiding this comment

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

Let's put the command on its own separate line. @sindresorhus said:

Having the command on a separate line makes it easier to copy-paste for users.
#1138 (comment)

*/

function getUpdateCommand(): ?string {
// Tarball install
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this inferrence, let's save a token somewhere as part of the packaging process. @cpojer said:

Can we somehow add a token to every build indicating the install method? I think that's way better than any sort of inference we might do.

My idea for that was to add an extra installationMethod field to package.json when building the packages. For example, something like this would be bundled in the Debian package:

{
  "name": "yarn",
  "version": "0.16.2",
  "installationMethod": "deb",

What do you think? #942 is the issue that covers(-ish) that. If you agree that that's a good idea, let's complete #942 before this one, and then rebase this on top of that.

The main issue I see with inference is when the user has multiple installations of Yarn. That's a bit silly, but they might have installed via the tarball and then also done npm install -g yarn for some reason. The instructions might tell them to update the wrong one.


function getUpdateCommand(): ?string {
// Tarball install
if (fs2.existsSync(path.join(userHome, '.yarn'))) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't ~/.yarn where user settings are stored?


function getUpdateInstaller(): ?string {
// Windows
if (fs2.existsSync('C:/Program Files/Yarn') || fs2.existsSync('C:/Program Files (x86)/Yarn')) {
Copy link
Member

@Daniel15 Daniel15 Oct 25, 2016

Choose a reason for hiding this comment

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

This won't work as the user can select any directory during installation. Using the environment variable (%ProgramFiles%\Yarn) is better, but still not 100% accurate. They might decide to install it to c:\apps\Yarn for example. There's a registry key we can read to get the path, but modifying package.json to have "installationMethod": "msi" as part of the build process would be the easiest thing to do, then we can just require(__dirname + 'package.json').installationMethod

} else {
const installer = getUpdateInstaller();
if (installer) {
this.reporter.info(this.reporter.lang('yarnOutdatedInstaller', installer));
Copy link
Member

Choose a reason for hiding this comment

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

Optional, but can we use something like boxen (or just render the ANSI codes ourself) to render a fancy box around it, like update-notifier does?

@Daniel15
Copy link
Member

See #1557. You should be able to build on that and just have a map of installationMethod => instructions 😄

sebmck pushed a commit that referenced this pull request Nov 1, 2016
* Add installationMethod property to package.json

References #1139, #942, #1429, #1138

* Make set-installation-method.js executable
printf "$red> Remove it (rm -rf ~/.yarn) and run this script again.$reset\n"
exit 0
if [ -n `which yarn` ]; then
LATEST_VERSION=`curl https://yarnpkg.com/latest-version`
Copy link
Member

Choose a reason for hiding this comment

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

This may need to handle the --nightly flag. We have this in yarn_get_tarball:

  if [ "$1" = '--nightly' ]; then
    url=https://nightly.yarnpkg.com/latest.tar.gz
  else
    url=https://yarnpkg.com/latest.tar.gz
  fi

You can use https://nightly.yarnpkg.com/latest-tar-version to get the latest nightly version.

return 'curl -o- -L https://yarnpkg.com/install.sh | bash';
}

if (YARN_INSTALL_METHOD === 'homebrwe') {
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "homebrwe" -> "homebrew"

}

if (YARN_INSTALL_METHOD === 'deb') {
return 'sudo apt-get updat e&& sudo apt-get install yarn';
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "updat e" -> "update"

if [ "$LATEST_VERSION" -eq "$YARN_VERSION" ]; then
printf "$green> Yarn is already at the latest version.$reset\n"
else
rm -rf "$HOME/.yarn"
Copy link
Member

Choose a reason for hiding this comment

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

2scary4me, maybe warn the user before doing this.

"A newer version of Yarn is available, do you want to delete the existing version and upgrade? (Y/N)"

Copy link
Member

Choose a reason for hiding this comment

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

Also won't this blow away their config? Or is that stored elsewhere?

if (YARN_INSTALL_METHOD === 'npm') {
return 'npm upgrade --global yarn';
}

Copy link
Member

Choose a reason for hiding this comment

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

Also add Chocolatey: choco upgrade yarn

return;
}

// don't bug for updates on tagged releases
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder if we should bug on nightly builds?

@@ -77,6 +77,10 @@ const messages = {
jsonError: 'Error parsing JSON at $0, $1.',
noFilePermission: "We don't have permissions to touch the file $0.",

yarnOutdated: "Your current version of Yarn is out of date. The latest version is $0 while you're on $1.",
yarnOutdatedInstaller: 'To upgrade, download the latest installer at $0.',
Copy link
Member

Choose a reason for hiding this comment

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

This is totally optional, but we could download the installer and run it for the user. Maybe we'll hold off on that for now and do it as an enhancement!

@Daniel15
Copy link
Member

Daniel15 commented Nov 4, 2016

Thanks @kittens! This looks pretty good to me. Just some small comments inline 😄

const path = require('path');
const fs2 = require('fs');

const {verison: YARN_VERSION, installationMethod: YARN_INSTALL_METHOD} = require('../../../package.json');
Copy link

Choose a reason for hiding this comment

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

possibly typo verison

@bestander
Copy link
Member

ping @kittens

@bestander bestander merged commit 15d0112 into master Nov 14, 2016
@wyze wyze deleted the update-check branch November 14, 2016 20:25
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.

None yet

5 participants