Use update-notifier to notify of new versions #990

[beardedvikingdev]

PR checklist

• Use update-notifier to notify users of new versions in a non-intrusive way Use update-notifier to notify users of new versions in a non-intrusive way #990
• New feature, bugfix, or enhancement
• Includes tests
• Documentation update

What changes did you make?

I've added the functionality of the update-notifier to have it display the update message when there is a new tslint version available.

The functionality is inside a new file called 'updateNotifier.ts', to keep it separate and because I need to require the plugin as well as the package.json. The new function is implemented via the cli.

Is there anything you'd like reviewers to focus on?

Do you think implementing the function call at this level is correct?

My reasons for placing it here is because it then runs every time. The interval for checking a new version is set to three days (default is once a day).

I've tested it by setting the 'updateCheckInterval' to 0 and by lowering the version inside the package.json to '3.15.0', so it would trigger the check.

This triggers the image below:

[palantirtech]

Thanks for your interest in palantir/tslint, @davieschoots! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[nchen63]

Nice. This may not work well with apps that consume tslint via a special formatter. For example, if the JSON formatter was used, the output would become invalid. Can you make the check only occur with prose output format?

[beardedvikingdev]

Hi @nchen63, sure no problem. I've moved and pushed the change and think this is what you wanted.

[nchen63]

it looks like update-notifier asks users to install using -g, but the running tslint may not be a global install

[beardedvikingdev]

Yes, there is a default option message which we can override. I'm happy to add this if you want. What would be your suggestion as text or do you think it's not desirable just to get rid of the -g?

[nchen63]

I'd prefer to point people to the change log since there may be breaking changes that require more than doing an npm install.

Also, I have a concern about whether this will work in Windows since I saw unix type commands in the referenced libraries

[nchen63]

also one of the files is missing the license header

[beardedvikingdev]

Sorry, I'll copy the license header and update the new file.

About the message, we could do something like below;
'There is a new update of tslint, please check our change log before updating'

I don't think it won't work on windows, but will definitely test it to see if it works.

Also I can remove the require for package.json, then use tslint as a fixed name value and get the version from Linter.VERSION instead of using it from the package.

[nchen63]

not working in windows is a deal breaker

[beardedvikingdev]

Yes, it sure is. I'm setting up a test right now :)

[beardedvikingdev]

Hi @nchen63 , I've did a test on windows 10 on cmd and powershell and it looks good.

[nchen63]

Can you use the text:

TSLint update available vX.Y.Z → vA.B.C
See https://github.com/palantir/tslint/blob/master/CHANGELOG.md

the url should be updated to a better place like https://palantir.github.io/tslint/ eventually, but we don't have our change log in the docs

[nchen63]

don't need this comment

[nchen63]

add license header

[IllusionMH]

Also looks good in Git Bash 👍

Is it possible to detect is it local (from dependencies or devDependencies) or global installation to provide proper update instruction (message option)? Since updating global won't fix outdated version on CI. (already mentioned in comments earlier)

Works fine if TSLint is launched like ./node_modules/.bin/tslint "src/**/*.ts", but won't any notification if launched from npm script (see sindresorhus/update-notifier#92).

[IllusionMH]

Looks like this string and rest of the file after it have redundant 1 space indentation.

[beardedvikingdev]

I've added all the changes:

• removed extra spaces
• added license
• added the desired text for when an update is available

I'll still want to do some final testing on windows (tonight or in the morning)

[beardedvikingdev]

@nchen63 I tested it on windows in powershell, cmd and git bash.

This is my first PR in a large codebase and I want to thank you for your patience!

@IllusionMH Thanks for testing and the review!

[nchen63]

very nice. Thanks for the contribution. Happy to help.