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: #2360. Add validation for project name. #2385

Closed
wants to merge 4 commits into from

Conversation

sunkehappy
Copy link

@sunkehappy sunkehappy commented Mar 5, 2021

Project name should not contain special characters like space. So we can exit when no suitable name provided.

@@ -31,6 +31,7 @@ const TEMPLATES = [
const renameFiles = {
_gitignore: '.gitignore'
}
const projectNameRE = /^[A-Za-z0-9_-]*$/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally incorrect. Please use something like validate-npm-package-name

Vite depends on resolve.exports which wouldn't pass your regex, for example.

@@ -31,6 +32,7 @@ const TEMPLATES = [
const renameFiles = {
_gitignore: '.gitignore'
}
const projectNameRE = /^[A-Za-z0-9_-]*$/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const projectNameRE = /^[A-Za-z0-9_-]*$/

This is no longer used.

Comment on lines 49 to 56
const validateResult = validatePackageName(name)
if (!validateResult.validForNewPackages) {
console.error(
// Only one name to be validated, so there will be only one error
validateResult.errors.length && validateResult.errors[0]
)
process.exit(1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const validateResult = validatePackageName(name)
if (!validateResult.validForNewPackages) {
console.error(
// Only one name to be validated, so there will be only one error
validateResult.errors.length && validateResult.errors[0]
)
process.exit(1)
}
const { errors } = validatePackageName(name);
if (errors) {
errors.unshift(`Invalid package name: ${name}`);
console.error(errors.join('\n - '));
process.exit(1);
}

While only one name will be validated, multiple errors can in fact be returned. For example, the name 'foobar ' will result in both name cannot contain leading or trailing spaces and name can only contain URL-friendly characters being returned. Multiple errors.

Copy link
Author

Choose a reason for hiding this comment

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

@rschristian Thank you. I correct it now.

@yyx990803
Copy link
Member

Thanks for the PR, but validate-npm-package-name pulls in a number of extra transitive dependencies which should ideally be avoided.

Also, we should allow user to create directories with any name, we just need to make sure the name used in package.json is valid so that npm/yarn doesn't complain. see 1dbf246

@yyx990803 yyx990803 closed this Mar 15, 2021
@rschristian
Copy link
Contributor

rschristian commented Mar 15, 2021

@yyx990803 Apologies, validate... was my suggestion, but how exactly does it pull in a number of transitive deps? The only item in the dep stack is builtins. http://npm.anvaka.com/#/view/2d/validate-npm-package-name

Fair call on allowing any directory name, though 1dbf246 is not a valid solution as npm/yarn still won't appreciate users using fs as their package name. The regex also allows me to make my package name .example, yet this too is invalid. <-- Oops, bad copy/paste to repl

I'd say it's probably beneficial to allow the lib which covers all edge cases to do this. Simpler than refining that regex and offloads the responsibility.

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.

4 participants