Cli: [BUG] Some packages can no longer be installed with npm v7

[debel27]

Current Behavior:

Some packages can no longer be installed with NPM v7 (in my case, it's [email protected])

To me, the install fails for a pretty legitimate reason. Yet, this behavior is not backward compatible.

Here is the relevant log

npm verb stack Error: Invalid tag name ">=^16.0.0": Tags may not have any characters that encodeURIComponent encodes.
npm verb stack     at invalidTagName (/usr/lib/node_modules/npm/node_modules/npm-package-arg/npa.js:91:15)
npm verb stack     at fromRegistry (/usr/lib/node_modules/npm/node_modules/npm-package-arg/npa.js:296:13)
npm verb stack     at Function.resolve (/usr/lib/node_modules/npm/node_modules/npm-package-arg/npa.js:81:12)
npm verb stack     at Arborist.[nodeFromEdge] (/usr/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:680:22)
npm verb stack     at Promise.all.peerEdges.map.edge (/usr/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:778:48)
npm verb stack     at Array.map (<anonymous>)
npm verb stack     at Arborist.[loadPeerSet] (/usr/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:778:17)
npm verb stack     at (anonymous function).then.node (/usr/lib/node_modules/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js:692:32)
npm verb stack     at process._tickCallback (internal/process/next_tick.js:68:7)
npm verb cwd /home/user/Desktop/test
npm verb Linux 3.13.0-24-generic
npm verb argv "/usr/bin/node" "/usr/bin/npm" "i" "[email protected]" "-dd"
npm verb node v10.14.0
npm verb npm  v7.0.0-beta.7
npm ERR! code EINVALIDTAGNAME
npm ERR! Invalid tag name ">=^16.0.0": Tags may not have any characters that encodeURIComponent encodes.
npm verb exit 1

Expected Behavior:

npm install works flawlessly

Steps To Reproduce:

npm init -y
npm i [email protected]

[mjeanroy]

Hi,

This issue is due to a bug in cache.js, line 83, the ternary expression seems wrong:

const spec = args[0] +
-     (args[1] === undefined || args[1] === null ? `@${args[1]}` : '')
+     (args[1] === undefined || args[1] === null ? '' : `@${args[1]}`)

I would be happy to submit a PR fixing the issue ;)

[ruyadorno]

note: I can still reproduce the issue in v7.0.0-beta.9 - we're def going to be following up with this one 😊

[mjeanroy]

Hi @ruyadorno,

I made a mistake, my PR fixed a bug with npm cache add command and I made a mistake because the error message was the same.

I took the time to debug what happened here and it looks like [email protected] define react as a peer dependency with an invalid semver constraint: >=^16.0.0, instead it should be >=16.0.0 or ^16.0.0 (note that it has been fixed with [email protected] and the the peer dependency is now defined with >=16.0.0 constraint).

In this case, the tag name verification made by npm-package-args dependency fails here (both version and range variable are null, so the last else is executed).

I don't think this is really a bug since a non valid semver constraint was probably not officially supported with npm < 7, but the error message can probably be improved :)

[debel27]

I don't think this is really a bug since a non valid semver constraint was probably not officially supported with npm < 7, but the error message can probably be improved :)

I disagree. Again, I believe the error is valid but it shouldn't be improved, it should be removed altogether.

Packages that were installable with NPM v6 are no longer installable with NPM v7. This means upgrading NPM may break projects. This might have been fine if there were a clear migration procedure, but in this case there is none. There may be packages whose latest version contains an invalid semver specifier, so the developer cannot upgrade its dependency to a higher version that fixes the problem.

[isaacs]

So, the handling of this broken semver range is not actually a breaking change. What changed is that we handle peerDependencies _at all_, so any broken garbage in a peerDependencies set would have been overlooked in npm v6, and no longer is.

Observe:

$ cat package.json
{
  "dependencies": {
    "react": ">=^16.0.0"
  }
}

$ npm i     # <-- using latest v7 beta
npm ERR! code EINVALIDTAGNAME
npm ERR! Invalid tag name ">=^16.0.0": Tags may not have any characters that encodeURIComponent encodes.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/isaacs/.npm/_logs/2020-09-22T21_01_51_852Z-debug.log

$ node ~/dev/npm/cli-v6 i   # <-- my checkout of v6
npm ERR! code EINVALIDTAGNAME
npm ERR! Invalid tag name ">=^16.0.0": Tags may not have any characters that encodeURIComponent encodes.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/isaacs/.npm/_logs/2020-09-22T21_01_59_215Z-debug.log

A more extreme example:

$ cat package.json
{
  "peerDependencies": {
    "react": "this is not a valid semver or dist-tag !$@!#$%#@$%!@#$!@#$%"
  }
}

$ npm i
npm ERR! code EINVALIDTAGNAME
npm ERR! Invalid tag name "this is not a valid semver or dist-tag !$@!#$%#@$%!@#$!@#$%": Tags may not have any characters that encodeURIComponent encodes.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/isaacs/.npm/_logs/2020-09-22T21_06_30_064Z-debug.log

$ node ~/dev/npm/cli-v6 i
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN bad-dep-version requires a peer of react@this is not a valid semver or dist-tag !$@!#$%#@$%!@#$!@#$% but none is installed. You must install peer dependencies yourself.
npm WARN bad-dep-version No description
npm WARN bad-dep-version No repository field.
npm WARN bad-dep-version No license field.

up to date in 0.307s
found 0 vulnerabilities


$ npm i --legacy-peer-deps

up to date in 245ms

found 0 vulnerabilities

I'm tempted to say that this is working as designed. If you want npm v7 to work when peerDependencies contain invalid data, you can use the --legacy-peer-deps switch to tell npm v7 to not look at peerDependencies at all.

The only alternative here would be to catch _any_ invalid data in peerDependencies (but _only_ in peerDependencies), and ignore those peer deps. So then we'd be in a situation where the contract that we install peerDeps is violated.

Another alternative would be that we figure out what >=^16.0.0 means, and add support for that syntax in node-semver. But now we're playing whack-a-mole with whatever unfiltered garbage happened to land in that previously ignored field.

Yes, packages using this dependency will break, but in this case, I don't really know if the alternatives are reasonable. Hopefully the users of react-tooltip can update to the fixed version swiftly.