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): project name with only numbers as an argument #4606

Merged

Conversation

task4233
Copy link
Contributor

@task4233 task4233 commented Aug 15, 2021

Description

If an argument for the project name is set only with numbers, path.join will be failed because the argument is retained in "number" type.

On entering the project name interactively, the type conversion to "string" type is implicitly performed because "trim" method is called in the middle.

I think it is strange that create-vite can generate a project with numbers on setting interactively, but not on setting it with arguments.

Additional context

The reason why happened that, it is automatically converted to "number" type, on not setting an option for "minimist", which is a library for managing arguments.
ref: https://github.com/substack/minimist/blob/aeb3e27dae0412de5c0494e9563a5f10c82cc7a9/index.js#L59-L61

I would like to attach the failed log for an reference.

$ yarn create vite 1234 --template react-ts
yarn create v1.22.11
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
success Installed "create-vite@2.5.4" with binaries:
      - create-vite
      - cva
[#########] 9/9TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type number (1234)
    at new NodeError (node:internal/errors:329:5)
    at validateString (node:internal/validators:129:11)
    at Object.join (node:path:1081:7)
    at init (/Users/task4233/.config/yarn/global/node_modules/create-vite/index.js:220:21) {
  code: 'ERR_INVALID_ARG_TYPE'
}
✨  Done in 0.66s.

Moreover, I would like to attach the succeeded log for an reference.
(2021-08-15 added)

$ yarn create vite --template react-ts
yarn create v1.22.11
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
success Installed "create-vite@2.5.4" with binaries:
      - create-vite
      - cva
✔ Project name: … 1234

Scaffolding project in /Users/task4233/work/1234...

Done. Now run:

  cd 1234
  yarn
  yarn dev

✨  Done in 4.18s.

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.

…umber when an user input number-only project name
patak-dev
patak-dev previously approved these changes Aug 15, 2021
@@ -3,7 +3,7 @@
// @ts-check
const fs = require('fs')
const path = require('path')
const argv = require('minimist')(process.argv.slice(2))
const argv = require('minimist')(process.argv.slice(2), { string: ['_'] })
Copy link
Member

Choose a reason for hiding this comment

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

I found it's hard to understand what's going on. We'd better add a comment for that, or just use toString() on usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antfu
Thanks for your comment!

I agree with it's hard to undarstand what's going on. So, I've added a comment for that.
Could you review that?

Thanks.

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 17, 2021
The comment before change was not helpful for understanding why option ( _ ) needs.

Co-authored-by: patak <matias.capeletto@gmail.com>
@patak-dev patak-dev changed the title fix(create-vite): fix bug on passing a project name with only numbers as an argument fix(create-vite): project name with only numbers as an argument Aug 18, 2021
@patak-dev patak-dev merged commit 085a621 into vitejs:main Aug 18, 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
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants