Suggest yarn when installed with yarn #132
Conversation
LitoMore
changed the title
|
This is not what was suggested in #118 (comment) This is not going to be accepted if it requires any |
index.js
Outdated
|
|
||
| try { | ||
| const realpath = fs.realpathSync(which().sync(cliName, {nothrow: true})); | ||
| if (realpath.match(/yarn\/global/)) { |
There was a problem hiding this comment.
I could be wrong, but this literal RegExp doesn't look to be Windows-friendly (hardcoded forward slash).
Perhaps we could simplify this to:
return realpath.includes(path.join('yarn', 'global'));There was a problem hiding this comment.
Thanks, I will try it in Windows.
|
@sindresorhus Thank you for your advice.
|
index.js
Outdated
| notify(opts) { | ||
| if (!process.stdout.isTTY || isNpm() || !this.update) { | ||
| return this; | ||
| } | ||
|
|
||
| opts = Object.assign({isGlobal: isInstalledGlobally()}, opts); | ||
| const installCommand = this.isYarn() ? 'yarn global upgrade ' + this.packageName : 'npm i ' + (opts.isGlobal ? '-g ' : '') + this.packageName; |
There was a problem hiding this comment.
We should probably do the global check on yarn too no?
|
Should we create a new package |
index.js
Outdated
| @@ -107,15 +107,24 @@ class UpdateNotifier { | |||
| }; | |||
| }); | |||
| } | |||
| isYarnGlobal() { | |||
| const isWindows = process.platform === 'win32' || process.env.OSTYPE === 'cygwin' || process.env.OSTYPE === 'msys'; | |||
There was a problem hiding this comment.
Should only be const isWindows = process.platform === 'win32';
index.js
Outdated
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| notify(opts) { | ||
| if (!process.stdout.isTTY || isNpm() || !this.update) { | ||
| return this; | ||
| } | ||
|
|
||
| opts = Object.assign({isGlobal: isInstalledGlobally()}, opts); |
There was a problem hiding this comment.
isInstalledGlobally already supports Yarn, but it doesn't return which one it is. For that, you could use https://github.com/sindresorhus/global-dirs#packages
There was a problem hiding this comment.
@sindresorhus I created a package is-yarn-global, it without any fs calls and dependencies. What about this?
index.js
Outdated
| notify(opts) { | ||
| if (!process.stdout.isTTY || isNpm() || !this.update) { | ||
| return this; | ||
| } | ||
|
|
||
| opts = Object.assign({isGlobal: isInstalledGlobally()}, opts); | ||
| const installCommand = this.isYarnGlobal() ? 'yarn global upgrade ' + this.packageName : 'npm i ' + (opts.isGlobal ? '-g ' : '') + this.packageName; |
There was a problem hiding this comment.
yarn global upgrade <package> will not upgrade package to latest version. Should be yarn global add <package> or yarn global upgrade <package>@latest.
|
@sindresorhus I could be wrong, I think is-yarn-global is better than global-dirs, |
Good point, yes, go for |
|
@sindresorhus |
|
@sindresorhus Code updated, please review |
|
ping @SBoudrias @sindresorhus I updated my code, but the review status does not change. Any progress with this pull request? |
|
@sindresorhus Hey Sindre! Please review! Thanks! |
|
Any update on this? |
|
This got lost in my review queue. @LitoMore Can you fix the merge conflicts? |
|
@sindresorhus Code updated. |
index.js
Outdated
| ...options | ||
| }; | ||
| const installCommand = options.isYarnGlobal ? `yarn global add ${this.packageName}` : `npm i ${options.isGlobal ? '-g ' : ''}${this.packageName}`; |
There was a problem hiding this comment.
Shouldn't we also show local Yarn command?
There was a problem hiding this comment.
Shouldn't we also show local Yarn command?
@sindresorhus How do we know if it is using Yarn or NPM locally?
There was a problem hiding this comment.
|
@sindresorhus Code updated. |
LitoMore
changed the title
|
current path is yarn project not mean this cli install by current local should not
|
Fixes #118
Resolve #146