[BUG] `npm version --save` drop workspace depending on alphabetical order

louis-bompart commented 2 years 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

In one project, with 3 workspaces a, b, c (names matter) with:

a depending on [b]
c depending on [a and b]

Run: npm version -w=b --save patch

The package.json of c (the 'root') is not updated, and c now depends on the registry version of b instead of the workspace/local version.

Now, with these dependency conditions instead:

a depending on [b and c]
b depending on [c]

Run: npm version -w=c --save patch (to continue bumping the 'leaf' of the dependency tree)

The package.json of a (the root) is updated, and a still depends on the workspace/local version.

Expected Behavior

Regardless of the package names, when running npm version -w=somepackagename --save patch, I expect all the workspace of the project to be updated.

Steps To Reproduce

Clone or reproduce: git clone https://github.com/louis-bompart/npm-version-ws-mrc.git
Run npm version -w=b patch --save --loglevel=silly

npm timing arborist:ctor Completed in 0ms
npm timing arborist:ctor Completed in 0ms
npm timing idealTree:init Completed in 17ms
npm timing idealTree:userRequests Completed in 0ms
npm sill idealTree buildDeps
npm timing idealTree:#root Completed in 0ms
npm timing idealTree:a Completed in 0ms
npm http fetch GET 200 https://registry.npmjs.org/b 15ms (cache hit)
npm sill placeDep c b@1.0.0 OK for: c@1.0.0 want: 1.0.0
npm timing idealTree:c Completed in 21ms

Environment

npm: v9.
Node.js: v16.16.0
OS Name: W10
npm config:
```
; "default" config from default values
```

_auth = (protected) access = null all = false allow-same-version = false also = null audit = true audit-level = null auth-type = "web" before = null bin-links = true browser = null ca = null cache = "C:\Users\louis\AppData\Local\npm-cache" cache-max = null cache-min = 0 cafile = null call = "" cert = null ci-name = null cidr = null color = true commit-hooks = true depth = null description = true dev = false diff = [] diff-dst-prefix = "b/" diff-ignore-all-space = false diff-name-only = false diff-no-prefix = false diff-src-prefix = "a/" diff-text = false diff-unified = 3 dry-run = false editor = "C:\Windows\notepad.exe" engine-strict = false fetch-retries = 2 fetch-retry-factor = 10 fetch-retry-maxtimeout = 60000 fetch-retry-mintimeout = 10000 fetch-timeout = 300000 force = false foreground-scripts = false format-package-lock = true fund = true git = "git" git-tag-version = true global = false global-style = false globalconfig = "C:\Users\louis\AppData\Roaming\npm\etc\npmrc" heading = "npm" https-proxy = null if-present = false ignore-scripts = false include = [] include-staged = false include-workspace-root = false init-author-email = "" init-author-name = "" init-author-url = "" init-license = "ISC" init-module = "C:\Users\louis\.npm-init.js" init-version = "1.0.0" init.author.email = "" init.author.name = "" init.author.url = "" init.license = "ISC" init.module = "C:\Users\louis\.npm-init.js" init.version = "1.0.0" install-links = true install-strategy = "hoisted" json = false key = null legacy-bundling = false legacy-peer-deps = false link = false local-address = null location = "user" lockfile-version = null loglevel = "notice" logs-dir = null logs-max = 10 ; long = false ; overridden by cli maxsockets = 15 message = "%s" metrics-registry = "https://registry.npmjs.org/" node-options = null noproxy = [""] offline = false omit = [] omit-lockfile-registry-resolved = false only = null optional = null otp = null pack-destination = "." package = [] package-lock = true package-lock-only = false parseable = false prefer-offline = false prefer-online = false ; prefix = "C:\Program Files\nodejs" ; overridden by builtin preid = "" production = null progress = true proxy = null read-only = false rebuild-bundle = true registry = "https://registry.npmjs.org/" replace-registry-host = "npmjs" save = true save-bundle = false save-dev = false save-exact = false save-optional = false save-peer = false save-prefix = "^" save-prod = false scope = "" script-shell = null searchexclude = "" searchlimit = 20 searchopts = "" searchstaleness = 900 shell = "C:\Windows\system32\cmd.exe" shrinkwrap = true sign-git-commit = false sign-git-tag = false strict-peer-deps = false strict-ssl = true tag = "latest" tag-version-prefix = "v" timing = false tmp = "C:\Users\louis\AppData\Local\Temp" umask = 0 unicode = true update-notifier = true usage = false user-agent = "npm/{npm-version} node/{node-version} {platform} {arch} workspaces/{workspaces} {ci}" userconfig = "C:\Users\louis\.npmrc" version = false versions = false viewer = "browser" which = null workspace = [] workspaces = null workspaces-update = true yes = null

; "builtin" config from C:\Users\louis\AppData\Roaming\npm\node_modules\npm\npmrc

prefix = "C:\Users\louis\AppData\Roaming\npm"

; "user" config from C:\Users\louis.npmrc

//localhost:4873/:_authToken = (protected) //registry.npmjs.org/:_authToken = (protected)

; "cli" config from command line options

long = true

louis-bompart commented 2 years ago

Some findings with the faulty (1st) scenario:

During the loading of the tree, Edges from c to b are marked as INVALID
During the loading of the tree, Edges from a to b are marked as OK
During the building of the ideal tree, a->b is processed before c->b.
- It adds b to the linkNodes Set
A solution then could be to modify the problemEdges by adding this condition early:
```
if (this[_linkNodes].has(edge.to)) {
return false
}
```

However, I think the real oddity is that c->b is INVALID when a->b is OK and that's probably the real underlying issue.

joeltg commented 1 year ago

I finally found this bug after days of debugging my workspaces setup, and if I understand it right, this completely breaks the ability to use the NPM CLI to version and publish a monorepo of packages that depend on each other. Is anyone with more context and knowledge about NPM internals able to take a look at this?

joeltg commented 1 year ago

Anyone able to take a look at this? Seems to be a deep bug that requires a lot of context to unravel

npm / cli