feat: use pnpm on projects that previously used pnpm #294
Conversation
|
@dylang could you have a look, please? |
lib/cli.js
Outdated
| detectPreferredPM(options.cwd) | ||
| ) | ||
| .then(preferredPM => { | ||
| options.installer = preferredPM && preferredPM.name === 'pnpm' ? 'pnpm' : 'npm'; |
There was a problem hiding this comment.
You are potentially checking .name of a string here 🤔
There was a problem hiding this comment.
Actually, not a super fan of this...
Could we do it like this:
- Change the default
installerfromnpmtoauto(on line 90/91)
installer: process.env.NPM_CHECK_INSTALLER || 'auto',
- Change the resolving to something like this:
Promise.resolve()
.then(() => {
if (options.installer === 'auto') {
return detectPreferredPM(options.cwd).then(result => result.name)
}
return options.installer
})
.then((installer) => {
return npmCheck(Object.assign(options, { installer }))
})
|
Saw some errors that should be fixed before merging |
|
@LinusU PR updated |
lib/cli.js
Outdated
| function detectPreferredInstaller(cwd) { | ||
| return detectPreferredPM(cwd) | ||
| .then(preferredPM => { | ||
| if (preferredPM && preferredPM.name === 'pnpm') { |
There was a problem hiding this comment.
I think it would be nice if this would accept e.g. yarn as well, and not specifically check for one manager...
There was a problem hiding this comment.
supporting yarn requires additional efforts because it has a different CLI. I just tested and got an error
Command failed: yarn install --save @types/[email protected] --color=always
error `install` has been replaced with `add` to add new dependencies. Run "yarn add @types/[email protected]" instead.
lib/cli.js
Outdated
| ( | ||
| options.installer === 'auto' ? | ||
| detectPreferredInstaller(options.cwd) : | ||
| Promise.resolve(process.env.NPM_CHECK_INSTALLER) |
There was a problem hiding this comment.
process.env.NPM_CHECK_INSTALLER should already have been read into options.installer, this will override any specific installer given by a CLI flag.
lib/cli.js
Outdated
| @@ -98,7 +99,15 @@ if (options.debug) { | |||
| debug('cli.input', cli.input); | |||
| } | |||
|
|
|||
| npmCheck(options) | |||
| ( | |||
There was a problem hiding this comment.
Personally not a fan of this style with a wide tenary and then chaining of that expression. What do you think about starting a Promise chain instead?
Promise.resolve()
.then(() => {
return (options.installer === 'auto' ? detectPreferredInstaller(options.cwd) : options.installer)
})This will catch any potential synchrounous throws and move them to a rejected Promise, and thus to the .catch handler further down.
If a project has a pnpm lockfile or it has a node_modules created by pnpm then pnpm is used to update dependencies close #291
| @@ -120,3 +130,13 @@ npmCheck(options) | |||
| } | |||
| process.exit(1); | |||
| }); | |||
|
|
|||
| var SUPPORTED_INSTALLERS = ['npm', 'pnpm', 'ied']; | |||
There was a problem hiding this comment.
The readme mentions ied, so I added it as well. Although, currently preferred-pm cannot detect it. I am not sure it ever will. Ied seems to be not maintained anymore
There was a problem hiding this comment.
Yarn can be added but it will require additional work. Yarn's CLI is not compatible with npm's
|
@LinusU updated |
|
should I change something? |
|
@dylang would you please merge/publish this? |
|
Published as 5.8.0, sorry for the delay |
If a project has a pnpm lockfile or it has a node_modules created
by pnpm then pnpm is used to update dependencies
close #291