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

feat(new): support to skip install #1458

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zanminkian
Copy link
Contributor

@zanminkian zanminkian commented Dec 17, 2021

If I run command nest new some-project (some one new may don't know --skip-install option), nest-cli always force me to select a package manager. I have to press ctrl + c to skip, which is not elegant. Otherwise I have to waste my time to wait it.

PR Checklist

Please check if your PR fulfills the following requirements:

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?

image

Issue Number: N/A

What is the new behavior?

image

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@@ -225,16 +218,18 @@ const printCollective = () => {
emptyLine();
};

const print = (color: string | null = null) => (str = '') => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prettier automatically format it.

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

LGTM

@kamilmysliwiec
Copy link
Member

Should we update this new option to say "none (skip installing packages)" instead of just "none" as one (newcomers to JS world) can think there's a "none" package manager? 😅

@zanminkian
Copy link
Contributor Author

Should we update this new option to say "none (skip installing packages)" instead of just "none" as one (newcomers to JS world) can think there's a "none" package manager? 😅

@kamilmysliwiec Good idea. Done now.

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,5 +1,6 @@
export enum PackageManager {
NPM = 'npm',
YARN = 'yarn',
PNPM = 'pnpm'
PNPM = 'pnpm',
NONE = 'none (skip installing packages)',
Copy link
Member

@micalevisk micalevisk Dec 20, 2021

Choose a reason for hiding this comment

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

I just notice something. If none is selected, then npm will be used by default on generated files (eg: README.md with usage commands) due to the following lines on application schematics

https://github.com/nestjs/schematics/blob/440d06d6217e40ca334be635d5264aac6dda8002/src/lib/application/application.factory.ts#L43-L46

thus, wouldn't be better if

Suggested change
NONE = 'none (skip installing packages)',
NONE = 'none (use NPM but without installing packages)',

or even

Suggested change
NONE = 'none (skip installing packages)',
NONE = 'NPM but without installing packages',

Copy link
Member

@micalevisk micalevisk Dec 20, 2021

Choose a reason for hiding this comment

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

actually, the selected option is not taken in count for now but this is bug that was already fixed by #1457, so you could think it is.

@zanminkian
Copy link
Contributor Author

@micalevisk I found it's not very elegant. I will convert this PR to draft.
The elegant way is asking user 2 questions:

  1. Which package manager would you like to use?
  • npm
  • yarn
  • pnpm
  1. Install packages now?
  • Yes
  • No

The first question is to generate README.md.
The second question is to skip installing or not.

What your opinion? @micalevisk @kamilmysliwiec @jmcdo29

@micalevisk
Copy link
Member

having two questions is better for sure.

@zanminkian
Copy link
Contributor Author

@micalevisk I found it's not very elegant. I will convert this PR to draft. The elegant way is asking user 2 questions:

  1. Which package manager would you like to use?
  • npm
  • yarn
  • pnpm
  1. Install packages now?
  • Yes
  • No

The first question is to generate README.md. The second question is to skip installing or not.

What your opinion? @micalevisk @kamilmysliwiec @jmcdo29

After PR #1457 being accepted, I will implement this feature.

@kamilmysliwiec
Copy link
Member

IMO the fewer questions we could ask the better (to make the scaffolding process as straightforward & quick as possible)

@zanminkian zanminkian marked this pull request as draft December 21, 2021 09:26
@zanminkian
Copy link
Contributor Author

IMO the fewer questions we could ask the better (to make the scaffolding process as straightforward & quick as possible)

Got it. And I found #1457 has code conflict with this PR. After that PR being accepted, I will rebase and ask for review again.

@micalevisk
Copy link
Member

to make the scaffolding process as straightforward & quick as possible

that's a good point acctually. Then, I go back to #1458 (comment) xD

@kamilmysliwiec
Copy link
Member

This should be feasible now (since we merged #1803)

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