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 ability to bypass isNpm check with shouldNotifyInNpmScript option #127

Merged
merged 6 commits into from
Apr 14, 2018

Conversation

alexccl
Copy link
Contributor

@alexccl alexccl commented Oct 18, 2017

My team and I have run into multiple scenarios where we've wanted to output the update message when running as an npm script. To do this, we've had to dig through the source code, hack environment variables, run this package, and reset environment variables.

This PR include functionality to bypass the isNpm() check. This will not break any existing functionality.


Fixes #122

Copy link
Contributor

@mischah mischah left a comment

Choose a reason for hiding this comment

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

This new option should be documented over here: https://github.com/yeoman/update-notifier#options

@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

index.js Outdated
@@ -38,6 +38,7 @@ class UpdateNotifier {
this.callback = options.callback || (() => {});
this.disabled = 'NO_UPDATE_NOTIFIER' in process.env ||
process.argv.indexOf('--no-update-notifier') !== -1;
this.skipIsNpmCheck = options.shouldNotifyInNpmScript;
Copy link
Owner

Choose a reason for hiding this comment

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

Better to give it the same name in both places:

this.shouldNotifyInNpmScript = options.shouldNotifyInNpmScript;

@sindresorhus sindresorhus changed the title Added ability to bypass isNpm with 'shouldNotifyInNpmScript' option Add ability to bypass isNpm check with shouldNotifyInNpmScript option Oct 30, 2017
@Aaronius
Copy link

Aaronius commented Apr 9, 2018

Is this waiting on something before being merged? I'd like to use it as well.

@SBoudrias
Copy link
Contributor

I'll go ahead and merge/release this.

@SBoudrias SBoudrias merged commit ac0d3cb into sindresorhus:master Apr 14, 2018
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.

5 participants