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

fix: create-vite when targetDir is a valid packageName #4247

Merged
merged 3 commits into from
Jul 14, 2021
Merged

fix: create-vite when targetDir is a valid packageName #4247

merged 3 commits into from
Jul 14, 2021

Conversation

spencer17x
Copy link
Contributor

Description

fix create-vite without packageName

Additional context

isValidPackageName judgment


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

The current code is doing what we intended, if the targetDir is a valid packageName, then the next question is ignored because it will be reused. We are only using that prompt if it is invalid.

@spencer17x would you create an issue to describe what problem did you run into?

@spencer17x
Copy link
Contributor Author

@patak-js Now when it is created, although the project name is valid, the name of package.json is still empty. When the type is null, the problem is skipped directly, resulting in packageName being undefined

@patak-dev
Copy link
Member

Good catch @spencer17x. The condition is still fine, I think we should modify this line

pkg.name = packageName
to fallback to targetDir if packageName is undefined.

@spencer17x
Copy link
Contributor Author

@patak-js I think so too, but considering that if the user expects the name of package.json to be inconsistent with the directory, this method requires the user to manually modify the name of package.json, so you think the method is better

@patak-dev
Copy link
Member

@patak-js I think so too, but considering that if the user expects the name of package.json to be inconsistent with the directory, this method requires the user to manually modify the name of package.json, so you think the method is better

We discussed this with the team last time, and it was decided that we should enable people to chose a directory name that is not a valid package.json name. We also don't want to ask too many prompts, only the questions that are strictly needed. So the current approach is fine, but you found a bug. You could use this PR if you would like to fix it as we discussed 👍🏼

@spencer17x
Copy link
Contributor Author

@patak-js So that's it, I understand, I modified it

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 14, 2021
@patak-dev patak-dev changed the title fix: fix create-vite without packageName fix: create-vite when targetDir is a valid packageName Jul 14, 2021
@patak-dev patak-dev merged commit 02e244d into vitejs:main Jul 14, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants