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-app): improved package name validation #2772

Merged
merged 4 commits into from
Mar 30, 2021

Conversation

Timsonrobl
Copy link
Contributor

@Timsonrobl Timsonrobl commented Mar 30, 2021

Description

This PR fixes some cases where current validation of package name in package.json introduced in 1dbf246 fails. Examples: names containing & , $ and @ not as part of scoped package name.
Silent package name substitution replaced with additional prompt that only appears if project name is cannot be used as legal package name. As such in most cases users won't be bothered with additional prompt but in edge cases they will be made immediately aware of possible issues with their choice of project name and given control over substitution, while maintaining the ease of using automated character substitution.

Additional context

Currently implied validation creates possibility of creating malformed package.json that would fail to initialize with yarn command with

error package.json: Name contains illegal characters

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.

packages/create-app/index.js Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

Looks great @Timsonrobl! Thanks for the PR

Note: While testing I saw that names like project& are converted to project-. We probably could trim that trailing -, but this is not related to this PR so I didn't include it in the review. Also, it is such an edge case that maybe is not worth taking into account.

@Shinigami92
Copy link
Member

suggestion (dx, if-minor): The packageName related stuff is already really huge
Should we consider to move it into its own function? Maybe to line 145 or below

  • That way we could just safe the variable name packageName and just wrap it in a function-call
  • Also it is more decoupled from the rest of the code

@Timsonrobl
Copy link
Contributor Author

Looks great @Timsonrobl! Thanks for the PR

Note: While testing I saw that names like project& are converted to project-. We probably could trim that trailing -, but this is not related to this PR so I didn't include it in the review. Also, it is such an edge case that maybe is not worth taking into account.

Is not perfect, it's quick and dirty. For example it would also change @scope/illegal&amp to _scope_illegal_amp but ideally it should be @scope/illegal_amp.
I don't think it can be done with just a replace() with regex though since JavaScript doesn't support lookbehind conditionals.

packages/create-app/index.js Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Mar 30, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

I want to turn on https://eslint.org/docs/rules/quotes later, IMO you can safely use single-quotes now

packages/create-app/index.js Show resolved Hide resolved
packages/create-app/index.js Show resolved Hide resolved
packages/create-app/index.js Show resolved Hide resolved
@Timsonrobl
Copy link
Contributor Author

Should quoting style changes be done in this PR though?

@Shinigami92
Copy link
Member

Should quoting style changes be done in this PR though?

-> #2772 (comment)

Seems no 🙂 🤷

@patak-dev patak-dev merged commit 9fa282e into vitejs:main Mar 30, 2021
@Timsonrobl Timsonrobl deleted the fix-package-name branch March 30, 2021 21:17
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.

None yet

3 participants