Skip to content

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

New issue
New issue
Open
Open
[BUG] 'v' prefix in package version causes incorrect arborist retirement#6370
Labels
Bugthing that needs fixingPriority 2secondary priority issueRelease 9.xwork is associated with a specific npm 9 release

Description

@silverwind
silverwind
opened on Apr 17, 2023

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • 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.

Activity

  1. silverwind
    added
    Bugthing that needs fixing
    Needs Triageneeds review for next steps
    Release 9.xwork is associated with a specific npm 9 release
    on Apr 17, 2023

  2. ljharb commented on Apr 17, 2023

    @ljharb
    ljharb
    on Apr 17, 2023
    Contributor

    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.

  3. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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 {}

  4. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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.

  5. ljharb commented on Apr 17, 2023

    @ljharb
    ljharb
    on Apr 17, 2023
    Contributor

    ah my bad, i misread your OP, sorry.

  6. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

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

  7. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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',

  8. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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

  9. ljharb commented on Apr 17, 2023

    @ljharb
    ljharb
    on Apr 17, 2023
    Contributor

    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.

  10. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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 anywhichway/nano-memoize#57 to work around the issue. Maybe later I'll check where arborist exactly goes wrong here.

  11. silverwind
    changed the title [-][BUG] Unexpected package re-installation with certain packages[/-] [+][BUG] 'v' prefix in package version causes incorrect arborist retirement[/+] on Apr 17, 2023

  12. silverwind commented on Apr 17, 2023

    @silverwind
    silverwind
    on Apr 17, 2023
    Author

    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 npm/node-semver#376 about that.

  13. silverwind commented on Apr 21, 2023

    @silverwind
    silverwind
    on Apr 21, 2023
    Author

    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.

  14. wraithgar
    added
    Priority 1high priority issue
    Priority 2secondary priority issue
    and removed
    Needs Triageneeds review for next steps
    Priority 1high priority issue
    on Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Bugthing that needs fixingPriority 2secondary priority issueRelease 9.xwork is associated with a specific npm 9 release

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions