Skip to content

Conversation

MLoughry
Copy link
Contributor

Fixes #6761

What's the problem this PR addresses?

By using - to separate the scope and name in slugifyident, the code could result in a name collision between packages (eg., @x-y/z and @x/y-z).

...

How did you fix it?

This change uses ~ instead, which is valid in file names but not package names.

...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@arcanis
Copy link
Member

arcanis commented Apr 10, 2025

Hm, it seems too wide a change to land in anything but a major; we use the slugify call in quite a few places.

Could we instead append the scope length to the metadata cache file name? The cache shouldn't be impacted since it's content-addressed.

@MLoughry
Copy link
Contributor Author

Might it make more sense to have a different function than slugifyIdent for the file name? Appending the scope length seems more hack-ish

@arcanis
Copy link
Member

arcanis commented Apr 11, 2025

Might it make more sense to have a different function than slugifyIdent for the file name?

I moved the function into npmHttpUtils to avoid expanding the public interface.

Appending the scope length seems more hack-ish

Yes and no - I still prefer that to a tilde, I'm worried that some filesystems or tools wouldn't play well with that character.

@arcanis arcanis changed the title Use ~ in slugifyIdent rather - Fixes the metadata file name to avoid ambiguities Apr 11, 2025
@arcanis arcanis merged commit 2394026 into yarnpkg:master Apr 11, 2025
26 checks passed
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.

[Bug?]: slugifyIdent in getPackageMetadata can cause a name collision for caching metadata
2 participants