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

npm package names should not be lowered #19

Merged

Conversation

pmalmsten
Copy link
Contributor

Like a few other package managers, npm package names are case sensitive. They now disallow uppercase characters in names for new packages (as discussed here), but older packages having uppercase names still exist (see https://www.npmjs.com/package/Acid vs. https://www.npmjs.com/package/acid).

So far I haven't seen evidence that scopes having uppercase characters exist, so I left that as-is.

npm used to support package names
containing capital characters:
https://blog.npmjs.org/post/168978377570/new-package-moniker-rules.html

Such packages still exist on the npm
registry, like this one:
https://www.npmjs.com/package/Acid/v/3.0.17
@pmalmsten
Copy link
Contributor Author

Hey @am11, mind taking a look at this? This bug is preventing our system from handling older npm packages properly, so we would love to get a fix in and a new version published to NuGet sooner rather than later.

Comment on lines 278 to 281
{
"nuget" or "cocoapods" or "cpan" or "vsm" or "cran" => name,
"nuget" or "cocoapods" or "cpan" or "vsm" or "cran" or "npm" => name,
"pypi" => name.Replace('_', '-').ToLower(),
_ => name.ToLower()
Copy link
Collaborator

@am11 am11 Oct 27, 2022

Choose a reason for hiding this comment

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

Should we align name and namespace validations with python implementation: https://github.com/package-url/packageurl-python/blob/6c96667/src/packageurl/__init__.py#L94? i.e. ToLower() transformation is not applied unless we are sure it's necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point - I'd be happy to contribute that next week, if you don't mind that going in as a separate PR.

Copy link
Collaborator

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Sorry, I missed it earlier. 😄

LGTM, other than a question/comment; which can be tackled in a separate PR.

I think it'd be good to align the normalization logic across the organization, and have some tests in shared test data.

@pmalmsten
Copy link
Contributor Author

Thanks @am11! Aligning to preserve-case-by-default is a great idea - I'd be happy to contribute that next week.

In the mean time, it would be helpful if we could be unblocked by getting this patch checked in and published - would you mind publishing this patch in a new version to NuGet? Thanks for your time looking at this.

@pmalmsten pmalmsten closed this Oct 28, 2022
@pmalmsten
Copy link
Contributor Author

Oops, I clicked the wrong button I think 😅

@pmalmsten pmalmsten reopened this Oct 28, 2022
@am11 am11 merged commit 810a771 into package-url:master Oct 28, 2022
@pmalmsten pmalmsten deleted the paulmalmsten/npm-names-are-case-sensitive branch October 28, 2022 20:13
@am11
Copy link
Collaborator

am11 commented Oct 28, 2022

v1.1.1 is published: https://www.nuget.org/packages/packageurl-dotnet/1.1.1

Thank you for the contribution. 👍

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.

2 participants