Skip to content

Conversation

@reznord
Copy link
Member

@reznord reznord commented Sep 7, 2017

This adds update notifier to the CLI and reminds the user on every CLI command her runs.

/cc @developit

@reznord reznord requested review from developit and lukeed September 7, 2017 11:39
@reznord
Copy link
Member Author

reznord commented Sep 7, 2017

Don't know how to test it locally lol 😛

Hoping that it will work 😅

src/index.js Outdated
import pkg from '../package.json';
import logo from './lib/logo';
import checkVersion from './../check';
import updateNotifier from 'update-notifier';
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 move this to line 2, so that it precedes all the local imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Didn't think about this earlier.

src/index.js Outdated
.strict()
.argv;

updateNotifier({pkg}).notify({ defer: true });
Copy link
Member

Choose a reason for hiding this comment

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

This should be before all the yargs(...) stuff takes place -- aka, right after the checkVersion().

Also, we don't need defer:true --- it's true by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/index.js Outdated

installHooks();

updateNotifier({pkg}).notify({ defer: true });
Copy link
Member

Choose a reason for hiding this comment

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

Can remove defer:true because it is already the default. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! shoot

Copy link
Member Author

Choose a reason for hiding this comment

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

Done 🙏

@lukeed lukeed merged commit da8e8ff into master Sep 8, 2017
@lukeed lukeed deleted the update-notifier branch September 8, 2017 06:34
@lukeed lukeed added this to the 2.0 milestone Sep 8, 2017
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.

3 participants