fix(plugin-npm): fix tag regex

[paul-soporan]

What's the problem this PR addresses?

The tag regex in @yarnpkg/plugin-npm doesn't include dots, which can cause problems.

Also, the tag regex in the ProtocolResolver from @yarnpkg/core doesn't even include numbers or dashes and is case sensitive, so some tags aren't remapped correctly.

Fixes #1267.

How did you fix it?

I unified the tag regexes into a single one exported from ProtocolResolver and I made it also include dots.

[bgotink]

There's no reason dist-tags can't start with v

As far as I can see there's two limitations to dist-tags:

• Tags may not have any characters that encodeURIComponent encodes. I copy pasted this from an npm error when trying to use a dist-tag with an @ in it. According to MDN that limits dist-tags to /[A-Za-z0-9_.!~*'()-]+/
• Tags that can be interpreted as valid semver ranges will be rejected. (cf. https://docs.npmjs.com/cli/dist-tag)

The string v't*e!s(t) is a valid dist tag:

$ npm --registry http://localhost:4873 add @foo/test@"v't*e!s(t)"
npm WARN test No description
npm WARN test No repository field.
npm WARN test No license field.

+ @foo/[email protected]
updated 1 package and audited 1 package in 0.825s
found 0 vulnerabilities

[arcanis]

The documentation recommends against:

The simplest way to avoid semver problems with tags is to use tags that do not begin with a number or the letter v.

Which ironically, was the case of the issue this PR closed (the problematic tag was starting with a number). I guess theoretically the foolproof way would be to do something like this:

^(?!v?[0-9]+(?:\.[0-9]+){2})

That starts to be pretty verbose though ...

[larixer]

the foolproof way would be to do something like this:

^(?!v?[0-9]+(?:\.[0-9]+){2})

That starts to be pretty verbose though ...

If we will do it this way we will match recommendation, but not the supported semantics which will break some packages that do not follow recommendation. Do we really want this?

[bgotink]

Or we could step away from regular expressions altogether?

function isValidTag(value: string) {
  return encodeURIComponent(value) === value && !semver.validRange(value);
}

[arcanis]

I find that overly broad. In particular, I'm considering a way to depend on workspaces by their name, and for now my candidate syntax is ~pkg-name, which would conflict with such a logic.

Frankly I'm fine with not supporting one-of-a-kind tags like v't*e!s(t), which are kinda asking for trouble anyway. The main thing is ensuring that semver versions aren't detected as tag... Maybe this?

const TAG_REGEXP = /^[a-z0-9][a-z0-9.-]*$/i;

function isValidTag(value: string) {
  return TAG_REGEXP.test(value) && !semver.valid(value);
}

[bgotink]

I see two different things here: npm dist-tags in general and package descriptors using a dist-tag but don't have a protocol.

For the npm: protocol I would consider it dangerous to deviate from what the npm registry provides. In other words: I would expect yarn add "@foo/test@npm:v't*e!s(t)" to work, even though it's an esoteric contrived example.

Using npm dist-tags without protocol can definitely be more strict, it's a shorthand that uses sane defaults most people will be happy with. If you need something different, e.g. because you're using an esoteric dist-tag, you can always specify the protocol explicitly.

This way we don't break any dependency—every valid npm dist-tag can be used in berry—but we give berry more freedom in the protocol resolver.

[paul-soporan]

I agree with @bgotink's idea.
Let's make the ProtocolResolver test tags using /^[a-z0-9-\.]+$/i or some other regex, while the NpmTagResolver can check using the function @bgotink proposed:

function isValidTag(value: string) {
  return encodeURIComponent(value) === value && !semver.validRange(value);
}

This way we can make the ProtocolResolver support more tag formats, as, for example, if we ever decide to create a PyPI Resolver / Fetcher (I've already played with the idea and created functioning Resolver and Fetcher prototypes a few days ago), Python uses a different version format (pep440), so using !semver.valid(value) might cause unepected problems for people that would set defaultProtocol to pypi: (or whatever other protocol would be used) for example.