-
Notifications
You must be signed in to change notification settings - Fork 557
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
feat: add a CLI message when newer version is available #292
Conversation
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'm really happy to see this is coming back :-) Suggested few minor things to check, happy to assist with any of them.
src/cli/index.ts
Outdated
// Checks for available update and returns an instance | ||
// Default updateCheckInterval is once a day | ||
var notifier = updateNotifier({ | ||
pkg: pkg, |
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.
You can write this as `updateNotifier({pkg}); when object key is same as a variable it points to, you can use this syntactic sugar :-)
src/cli/index.ts
Outdated
var notifier = updateNotifier({ | ||
pkg: pkg, | ||
}); | ||
// Notify using the built-in convenience method |
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'm not sure I understand this comment. Do you think we need it?
src/cli/index.ts
Outdated
import * as updateNotifier from 'update-notifier'; | ||
var pkg = require('../../package.json'); | ||
|
||
async function updateCheck() { |
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.
As notify is not asycnhronous (i.e. we don't do await notifier.notify()
, async
keyword before the function name is misleading.
src/cli/index.ts
Outdated
@@ -108,6 +121,7 @@ function checkPaths(args) { | |||
} | |||
|
|||
async function main() { | |||
await updateCheck(); |
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.
Related to the comment above, updateCheck()
can be fully synchronous.
src/cli/index.ts
Outdated
@@ -14,6 +14,19 @@ import errors = require('../lib/error'); | |||
import ansiEscapes = require('ansi-escapes'); | |||
import {isPathToPackageFile} from '../lib/detect'; | |||
|
|||
import * as updateNotifier from 'update-notifier'; | |||
var pkg = require('../../package.json'); |
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.
- var
+ const
src/cli/index.ts
Outdated
@@ -14,6 +14,19 @@ import errors = require('../lib/error'); | |||
import ansiEscapes = require('ansi-escapes'); | |||
import {isPathToPackageFile} from '../lib/detect'; | |||
|
|||
import * as updateNotifier from 'update-notifier'; | |||
var pkg = require('../../package.json'); |
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.
Personal sidenote - I don't like using require
to load .json files, I would suggest fs.readFile
, but I fear we can saw this pattern in this app more often, so it's probably good to go.
97112f1
to
061388b
Compare
061388b
to
1c2bd4a
Compare
src/cli/index.ts
Outdated
try { | ||
notifier = updateNotifier({pkgFile}); | ||
} catch (err) { | ||
console.warn('Failed to start updater, ERR=', err); |
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.
Not sure whether to keep this warning. The code works from different directories, errors were observed during the tests on travis only
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 imagine that we get here when version
is not present in the package.json as would be the case on Travis etc.
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.
exactly
return; | ||
} | ||
|
||
notifier.notify(); |
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.
notifier
could be null
here due to let
and try/catch
.. perhaps move notifier.notify();
up into the try block.
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 return from catch, but you're right, will move
1c2bd4a
to
5053c4e
Compare
🎉 This PR is included in version 1.116.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this PR do?
Adding a message to CLI when a newer version is available
What are the relevant tickets?
https://snyksec.atlassian.net/browse/SC-6399
Screenshots