[BUG] 'v' prefix in package version causes incorrect arborist retirement

silverwind commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

This issue exists in the latest npm version

[X] I am using the latest npm

Current Behavior

On some rare packages, npm seems to incorrectly re-install them when the timestamp of node_modules is updated and the second installation does not print "up to date" but "changed 1 package". The issue seems to be very specific to nano-memoize and I suspect their empty engines might have something to do with it.

Expected Behavior

Clone reproduction repo and try it:

git clone https://github.com/silverwind/nano-npm && cd nano-npm
rm -rf node_modules && npm i && touch node_modules && npm i

It should print

added 1 package in 97ms
up to date in 87ms

but it actually prints

added 1 package in 97ms
changed 1 package in 95ms

In the verbose log it can be seen that in the second run, there is a "retired":

npm info using npm@9.6.4
npm info using node@v19.8.1
npm verb title npm install
npm verb argv "install" "--no-save" "--loglevel" "silly"
npm verb logfile logs-max:0 dir:/Users/silverwind/.npm/_logs/2023-04-17T19_36_39_018Z-
npm verb logfile no logfile created
npm sill logfile done cleaning log files
npm sill idealTree buildDeps
npm sill reify moves {}
npm sill ADD node_modules/nano-memoize

added 1 package in 97ms
npm verb exit 0
npm info ok
npm verb cli /usr/local/Cellar/node/19.8.1/bin/node /Users/silverwind/.npm-global/bin/npm
npm info using npm@9.6.4
npm info using node@v19.8.1
npm verb title npm install
npm verb argv "install" "--no-save" "--loglevel" "silly"
npm verb logfile logs-max:0 dir:/Users/silverwind/.npm/_logs/2023-04-17T19_36_39_496Z-
npm verb logfile no logfile created
npm sill logfile done cleaning log files
npm sill idealTree buildDeps
npm verb shrinkwrap failed to load node_modules/.package-lock.json out of date, updated: node_modules
npm sill reify mark retired [ '/Users/silverwind/git/nano-npm/node_modules/nano-memoize' ]
npm sill reify moves {
npm sill reify   '/Users/silverwind/git/nano-npm/node_modules/nano-memoize': '/Users/silverwind/git/nano-npm/node_modules/.nano-memoize-mSgVARDY'
npm sill reify }
npm sill CHANGE node_modules/nano-memoize

changed 1 package in 95ms
npm verb exit 0
npm info ok

Steps To Reproduce

See above

Environment

See log output above.

ljharb commented 1 year ago

Do you mean, it downloads it from github a second time? I don't think packages from github can ever be cached, so they're always fetched fresh.

silverwind commented 1 year ago

No, these are normal npm registry packages. The only difference in the log of the unexpected reinstallation is the moves object being popuplated and mark retired is shown. E.g. with the repro:

npm sill reify mark retired [ '/Users/silverwind/git/nano-npm/node_modules/nano-memoize' ]
npm sill reify moves {
npm sill reify   '/Users/silverwind/git/nano-npm/node_modules/nano-memoize': '/Users/silverwind/git/nano-npm/node_modules/.nano-memoize-mSgVARDY'
npm sill reify }

With a different package that does not have the issue:

npm sill reify moves {}

silverwind commented 1 year ago

Some more observations:

Bug does not happen without a lockfile
Bug does not happen with other packages
Bug does not happen if node_modules is not touched.

ljharb commented 1 year ago

ah my bad, i misread your OP, sorry.

silverwind commented 1 year ago

Empty engines is not the reason. I've published the package without it, the issue persists. Will debug further.

silverwind commented 1 year ago

Checking further, it seems Arborist sees below diff. Only difference is that actual does not have a resolved property in the second run, despite it being present and matching below ideal in the first run after the ADD action.


actual: ArboristNode {
  name: '@silverwind/nano-memoize',
  version: 'v3.0.11',
  location: 'node_modules/@silverwind/nano-memoize',
  path: '/Users/silverwind/git/nano-npm/node_modules/@silverwind/nano-memoize',
  edgesIn: Set(1) { { "" prod @silverwind/nano-memoize@3.0.11 } }
},
ideal: ArboristNode {
  name: '@silverwind/nano-memoize',
  version: '3.0.11',
  location: 'node_modules/@silverwind/nano-memoize',
  path: '/Users/silverwind/git/nano-npm/node_modules/@silverwind/nano-memoize',
  resolved: 'https://registry.npmjs.org/@silverwind/nano-memoize/-/nano-memoize-3.0.11.tgz',
  edgesIn: Set(1) { { "" prod @silverwind/nano-memoize@3.0.11 } }
},
action: 'CHANGE',

silverwind commented 1 year ago

Found the root cause, the package has a v prefix in package.json's version field which then triggers the bug in arborist that leads to arborist incorrectly retiring the package:

https://github.com/anywhichway/nano-memoize/blob/d615ae143f8cca6b999e2d1342478442c347f3d1/package.json#L3

@silverwind/nano-memoize@3.0.16 is the last version with prefix
@silverwind/nano-memoize@3.0.17 is the first version without it

ljharb commented 1 year ago

I'm surprised a package with a v in that field would be accepted to be published by the registry. If that's the case, even for historical packages, then arborist may need to allow it.

silverwind commented 1 year ago

Docs say it must be parsable by semver which happily does it. So I guess it's not strictly invalid, but to avoid this bug and similar, it might be good that npm publish forbids it, I suppose.

> require("semver").parse("v1.2.3")
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: 'v1.2.3',
  major: 1,
  minor: 2,
  patch: 3,
  prerelease: [],
  build: [],
  version: '1.2.3'
}

I opened https://github.com/anywhichway/nano-memoize/pull/57 to work around the issue. Maybe later I'll check where arborist exactly goes wrong here.

silverwind commented 1 year ago

As per semver spec, a "v" prefix is invalid. But the semver package apparently does not honor that spec and strips the prefix.

Edit: found https://github.com/npm/node-semver/issues/376 about that.

silverwind commented 1 year ago

Found another affected package: https://github.com/BackendStack21/0http/blob/1934ce50b67cd5f89b482e3a60e615583875eb68/package.json#L3, so it looks it's not that uncommon that packages have this "v" prefix in their version field. Will file a PR there as well.

npm / cli