From 2720b87899c2b6b9fd56d68acdda57fab12cd731 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Thu, 25 Jun 2026 23:35:38 +0530 Subject: [PATCH] fix(arborist): correct dev/prod dep flags for workspaces under the linked strategy (#9655) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In continuation of our exploration of using `install-strategy=linked` in the [Gutenberg monorepo](https://github.com/WordPress/gutenberg/pull/75814), which powers the WordPress Block Editor. Under `install-strategy=linked`, `npm query` reports the wrong `dev`/`prod` flags for workspaces and their dependencies. In a workspace project the entire non-root tree is flagged `dev`, so `:is(.prod)` returns almost nothing and `:is(.dev)` returns almost everything — the opposite of the hoisted strategy. This breaks tooling that classifies dependencies via `npm query`, e.g. a license checker that selects `.prod` dependencies. ## Why Two compounding defects, both exercised only by the linked layout. First, the linked strategy does not symlink undeclared workspaces into the root's `node_modules`, so the root's `workspace` edges resolve to `null`. `calcDepFlags` walks outward from the root via edges, dead-ends immediately, and never reaches any workspace or its transitive deps, leaving them at their default `dev=true`. Second, the `node.isLink` branch in `calcDepFlags` assigned target flags unconditionally (`target.dev = link.dev`), unlike every other flag in that file which is only ever unset (true to false). When a target is reachable through more than one link — the norm under linked, where each workspace's own `node_modules` links to a shared target — the last link visited could overwrite an already-correct `dev=false` back to `true`. ## How Make the `calcDepFlags` link branch monotonic: only unset flags, matching the edge walk below it, and queue the target on first visit so its own deps are still walked. A target reachable through multiple links now keeps the most permissive flags regardless of visit order. In `loadActual`, when the install strategy is linked, synthesize the missing root-to-workspace links from the already-loaded workspace targets so the root's workspace edges resolve and flags propagate. The synthesis is gated to linked because under hoisted an unresolved workspace edge is a genuinely missing symlink that reify must recreate, not synthesize. Workspaces already linked into the root `node_modules` are skipped. This targets the path used by `npm query` and non-lockfile `npm sbom`, which force a filesystem read of the actual tree. Commands that load from the hidden lockfile (`npm ls`, `npm outdated`, `npm audit signatures`) are unchanged; their separate, pre-existing linked flag gap is left for a follow-up. ## References Fixes #9100 (cherry picked from commit f9e3a80a3bb8617bf4e8a9fef20c3ecd4f8d7928) --- .../arborist/lib/arborist/load-actual.js | 16 +++++++ workspaces/arborist/lib/calc-dep-flags.js | 32 +++++++++++--- .../arborist/test/arborist/load-actual.js | 43 +++++++++++++++++++ workspaces/arborist/test/calc-dep-flags.js | 31 +++++++++++++ 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index 7746495b683bb..711dd920435dc 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/workspaces/arborist/lib/arborist/load-actual.js @@ -91,6 +91,8 @@ module.exports = cls => class ActualLoader extends cls { transplantFilter = () => true, ignoreMissing = false, forceActual = false, + // always present: the public loadActual merges this.options, which sets installStrategy + installStrategy, } = options this.#filter = filter this.#transplantFilter = transplantFilter @@ -171,6 +173,20 @@ module.exports = cls => class ActualLoader extends cls { } } await Promise.all(promises) + + // Linked undeclared workspaces aren't symlinked into root node_modules, so their edges resolve to null and flags never propagate. + // Synthesize the links from the loaded targets. Gated to linked; under hoisted a null workspace edge is a real missing link. + if (installStrategy === 'linked') { + for (const [name, path] of this.#actualTree.workspaces.entries()) { + const edge = this.#actualTree.edgesOut.get(name) + // skip workspaces already linked into root node_modules (declared deps) + if (edge.to) { + continue + } + const target = this.#cache.get(path) + new Link({ parent: this.#actualTree, name, realpath: path, target, pkg: target.package }) + } + } } if (!ignoreMissing) { diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 5f2484858094d..965127f5ac9dd 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -35,12 +35,32 @@ const calcDepFlags = (tree, resetRoot = true) => { if (node.target == null) { continue } - node.target.dev = node.dev - node.target.optional = node.optional - node.target.devOptional = node.devOptional - node.target.peer = node.peer - node.target.extraneous = node.extraneous - queue.push(node.target) + // Only unset flags, matching the edge walk below, so multiple links to one target can't clobber a correct value. + let changed = false + if (node.target.dev && !node.dev) { + node.target.dev = false + changed = true + } + if (node.target.optional && !node.optional) { + node.target.optional = false + changed = true + } + if (node.target.devOptional && !node.devOptional) { + node.target.devOptional = false + changed = true + } + if (node.target.peer && !node.peer) { + node.target.peer = false + changed = true + } + if (node.target.extraneous && !node.extraneous) { + node.target.extraneous = false + changed = true + } + // queue target on first visit so its deps are walked + if (changed || !seen.has(node.target)) { + queue.push(node.target) + } continue } diff --git a/workspaces/arborist/test/arborist/load-actual.js b/workspaces/arborist/test/arborist/load-actual.js index c933d4f5fadf1..0f56c8ab64c1d 100644 --- a/workspaces/arborist/test/arborist/load-actual.js +++ b/workspaces/arborist/test/arborist/load-actual.js @@ -303,6 +303,49 @@ t.test('transplant workspace targets, even if links not present', async t => { }), 'do not transplant node named "a"') }) +t.test('linked strategy propagates dep flags into undeclared workspaces', async t => { + // Undeclared workspaces aren't in root node_modules under linked; without the fix their edges resolve to null and stay dev. + // `tools` is also a devDependency of `app`, so it is reached by both a prod root link and a dev `app` link. + // `app` is linked into root node_modules, so its edge already resolves and the synthesis is skipped. + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', + workspaces: ['packages/*'], + }), + node_modules: { + '@test': { + app: t.fixture('symlink', '../../packages/app'), + }, + }, + packages: { + app: { + 'package.json': JSON.stringify({ + name: '@test/app', + version: '1.0.0', + devDependencies: { '@test/tools': '*' }, + }), + node_modules: { + '@test': { + tools: t.fixture('symlink', '../../../tools'), + }, + }, + }, + tools: { + 'package.json': JSON.stringify({ + name: '@test/tools', + version: '1.0.0', + }), + }, + }, + }) + const tree = await loadActual(path, { installStrategy: 'linked' }) + const byLocation = loc => [...tree.inventory.values()].find(n => n.location === loc) + const app = byLocation('packages/app') + const tools = byLocation('packages/tools') + t.equal(app.dev, false, 'app workspace is prod') + t.equal(tools.dev, false, 'tools workspace is prod, not overwritten by the dev app link') +}) + t.test('load workspaces when loading from hidden lockfile', async t => { const path = t.testdir({ 'package.json': JSON.stringify({ diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index b371ae55f6e4b..70a5ec48b6a46 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -343,6 +343,37 @@ t.test('set parents to not extraneous when visiting', t => { t.end() }) +t.test('multiple links to one target keep the most permissive flags', async t => { + // `tools` is reached by a prod root link and a dev `app` link; the dev link must not overwrite the prod dev flag. + const root = new Node({ + path: '/r', + realpath: '/r', + pkg: { name: 'root', workspaces: ['app', 'tools'] }, + }) + const app = new Node({ + path: '/r/app', + realpath: '/r/app', + root, + pkg: { name: 'app', version: '1.0.0', devDependencies: { tools: '*' } }, + }) + const tools = new Node({ + path: '/r/tools', + realpath: '/r/tools', + root, + pkg: { name: 'tools', version: '1.0.0' }, + }) + new Link({ name: 'app', parent: root, realpath: '/r/app', target: app }) + new Link({ name: 'tools', parent: root, realpath: '/r/tools', target: tools }) + new Link({ name: 'tools', parent: app, realpath: '/r/tools', target: tools }) + root.workspaces = new Map([['app', '/r/app'], ['tools', '/r/tools']]) + + calcDepFlags(root) + + t.equal(tools.dev, false, 'tools is prod via the root workspace link') + t.equal(app.dev, false, 'app is prod via the root workspace link') + t.end() +}) + t.test('check null target in link', async t => { const root = new Link({ path: '/some/path',