nest-cli icon indicating copy to clipboard operation
nest-cli copied to clipboard

feat(new): check for installed package managers

Open onurravli opened this issue 1 year ago • 9 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [X] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [ ] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Currently, the CLI tool asks for all package managers (npm, yarn, and pnpm) even if they aren't installed.

What is the new behavior?

Now, CLI checks for installed package managers and then asks for those that are installed, not for the ones that are not installed.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

onurravli avatar Feb 03 '24 17:02 onurravli

Hey @kamilmysliwiec, can you check this out?

onurravli avatar Feb 04 '24 09:02 onurravli

Not sure how I feel about this change. Isn't this confusing that some options just won't show up for certain people? Like one might be aware that yarn is a thing but doesn't have it installed on let's say their machine at work. Then they generate a NestJS app and they only see NPM showing up which will make them think using yarn was never an option (simply because it's hidden).

Shouldn't we just leave this decision to the developer since the developer should be aware of what tool they have installed (and even if they don't they can always terminate the command, install it, and start over) and want to use for their projects?

kamilmysliwiec avatar Feb 05 '24 07:02 kamilmysliwiec

Yes, this is a bit of a "divisive" change, to one 50 percent it can be helpful while the other 50 percent can be confusing 😄

Tony133 avatar Feb 05 '24 07:02 Tony133

Not sure how I feel about this change. Isn't this confusing that some options just won't show up for certain people? Like one might be aware that yarn is a thing but doesn't have it installed on let's say their machine at work. Then they generate a NestJS app and they only see NPM showing up which will make them think using yarn was never an option (simply because it's hidden).

Shouldn't we just leave this decision to the developer since the developer should be aware of what tool they have installed (and even if they don't they can always terminate the command, install it, and start over) and want to use for their projects?

I made this change after I realize there is an error when you continue with yarn even yarn is not installed, as below.

image

If it's not a good idea to hide package managers, it might be a better idea to warn the user if, for example, yarn is not installed, but to build the project (without installing packages).

onurravli avatar Feb 05 '24 07:02 onurravli

Even if the installation fails, everything else should be properly generated and you should be good to go as long as you manually install dependencies

kamilmysliwiec avatar Feb 05 '24 07:02 kamilmysliwiec

Yes, I mean the error is not clear. In my firsr use of nest-cli, I thought project wasn't built.

onurravli avatar Feb 05 '24 07:02 onurravli

So maybe instead of hiding package managers we should rather improve our error messages; let users know that even if the installation failed they're generally (almost) all set

kamilmysliwiec avatar Feb 05 '24 08:02 kamilmysliwiec

Agree. Should I close this PR and open a new one, or continue on this?

onurravli avatar Feb 05 '24 08:02 onurravli

Feel free to continue in this PR

kamilmysliwiec avatar Feb 05 '24 08:02 kamilmysliwiec