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: two folders are created when the project name contains spaces #11630

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

btea
Copy link
Collaborator

@btea btea commented Jan 8, 2023

Description

When select customize with create-vue, If the project name contains a space, two folders will be created, and then the template content will be filled into the folder named with the character before the first space.

Additional context


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 PR Title 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.

@btea

This comment was marked as resolved.

Comment on lines 339 to 341
if (targetDir.includes(' ')) {
args.splice(2, args.length - 2, targetDir)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think simply replacing args instead of replacing the fullCustomCommand would be more easy to understand.

// we replace TARGET_DIR here because targetDir may include a space
let replacedArgs = args.map(arg => arg.replace('TARGET_DIR', targetDir))

Would you also add a test here?
https://github.com/vitejs/vite/blob/main/packages/create-vite/__tests__/cli.spec.ts

Copy link
Collaborator Author

@btea btea Jan 20, 2023

Choose a reason for hiding this comment

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

The code has been updated, but there seems to be something wrong with my device, could you help to add a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I add the test, I found that if projectName contains spaces, the value received by progress.argv will be split. Therefore, targetDir can only get the characters before the spaces. How to deal with this scense? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

In that case, I think the user should quote the project name (e.g. npm init vite@latest "project name with spaces").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, However, when executing pnpm create vite, the input Project name may contain spaces. I think that after the template is created, if the project name contains spaces, the project name should be wrapped in quotes in the cd projectName output from the terminal.

Copy link
Member

Choose a reason for hiding this comment

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

So you're suggest updating this line? I think that makes sense.

console.log(` cd ${path.relative(cwd, root)}`)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think it should be updated here. And the previous modification is also necessary. But I don't know how to write this test case?

Copy link
Member

Choose a reason for hiding this comment

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

I tried writing the test but it seems to be quite complex. I think we can go without tests.

If we replace this execaCommandSync with execaSync, we can test the case of passing the project name from argv.

return execaCommandSync(`node ${CLI_PATH} ${args.join(' ')}`, options)

But for the case of typing the project name, we need to feed the text from stdin and the test seems to hang.

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jan 21, 2023
@patak-dev patak-dev merged commit 758fc6c into vitejs:main Feb 1, 2023
@btea btea deleted the fix/create-vite-vue branch February 1, 2023 13:49
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