feat(cli): allow the ability to specify package manager in init command #6820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
No changes to documentation |
|
Component Testing Report Updated May 31, 2024 3:41 PM (UTC)
|
rexxars
previously approved these changes
May 30, 2024
Comment on lines
6
to
8
| import {type PackageManager} from '../../packageManager/packageManagerChoice' | ||
| import {type CliCommandDefinition} from '../../types' | ||
|
|
There was a problem hiding this comment.
Suggested change
| import {type PackageManager} from '../../packageManager/packageManagerChoice' | |
| import {type CliCommandDefinition} from '../../types' | |
| import {ALLOWED_PACKAGE_MANAGERS, type PackageManager} from '../../packageManager/packageManagerChoice' | |
| import {type CliCommandDefinition} from '../../types' | |
| const allowedPms = ALLOWED_PACKAGE_MANAGERS.map(pm => `"${pm}"`).join(' | ') |
Then use ${allowedPms} in the help string, maybe?
Would help keep them in line.
There was a problem hiding this comment.
good point, I can make the change
juice49
reviewed
May 31, 2024
| // If the user has specified a package manager, and it's allowed use that | ||
| if (packageManager && ALLOWED_PACKAGE_MANAGERS.includes(packageManager)) { | ||
| pkgManager = packageManager | ||
| } else { |
There was a problem hiding this comment.
It looks like if the user specifies a disallowed package manager then we silently fall back to an allowed one. Should we warn the user?
There was a problem hiding this comment.
can, what would the copy look like?
There was a problem hiding this comment.
Something like this?
output.print(`Given package manager "${packageManager}" isn't supported. Supported package managers are ${allowedPms}.`)
output.print(`Continuing using ${pkgManager}...`)There was a problem hiding this comment.
Does this make sense?
There was a problem hiding this comment.
That looks good to me!
(However, we should consider supporting dog).
juice49
reviewed
May 31, 2024
Co-authored-by: Ash <[email protected]>
juice49
approved these changes
May 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds
--package-managerflag to the init command. If passed and valid, it will use this flag as the package manager when installing dependencies.Note: I have not touched the nextjs init package manager choosing piece not sure if it should or not.
What to review
Changes and copy makes sense. You can still create new projects with and without this flag.
Testing
I manually tested different scenarios, this PR will unblock future PRs for testing the init command
Notes for release
--package-managerflag to init command to choose a package manager