From 5ed313158945ab0f7745f2d0d4a09e9e50c36253 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Thu, 25 Jun 2026 23:07:44 +0530 Subject: [PATCH] fix(query): report logical dep location under linked strategy (#9656) 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 ':root > *'` reported a direct dependency at its `node_modules/.store//node_modules/` backing path instead of the logical `node_modules/` location, diverging from the hoisted strategy. There are two root causes. In `hasParent` (query-selector-all.js), a store-backing node is `isTop`, has `resolveParent === root`, and has a top-level symlink whose parent is root, so it matched as a direct child of root through the `linksIn` logical-parent block that exists for workspaces. This returned the store node alongside its logical `Link`. In the `query` command, `QuerySelectorItem` read `node.target.location`, which for a linked dep resolves to the store node. The fix skips store-backing nodes in the `linksIn` parent check, so a store node is reached only through its logical `Link` (matched via the `edgesIn` branch) rather than as a direct child of root. The `query` command now reports the node's own logical `location`/`path` when the target is in the store, while workspaces and regular nodes keep the target location (for example `packages/`). Deduplication now keys on the target's physical location and ranks competing representations of the same package, so a top-level placement wins over the canonical store node, which wins over an internal store symlink. This keeps direct deps logical and transitive deps at their canonical store key for selectors such as `:root *`, and leaves the hoisted strategy unchanged. ## References Fixes #9617 (cherry picked from commit 803ba701d7a4f188308d99d47d1f5587930acbba) --- lib/commands/query.js | 38 ++++++++++--- test/lib/commands/query.js | 55 +++++++++++++++++++ workspaces/arborist/lib/query-selector-all.js | 3 +- .../arborist/test/query-selector-all.js | 7 +++ 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/lib/commands/query.js b/lib/commands/query.js index 63bd00c3206b3..f826cdc52659e 100644 --- a/lib/commands/query.js +++ b/lib/commands/query.js @@ -2,6 +2,15 @@ const { resolve } = require('node:path') const BaseCommand = require('../base-cmd.js') const { log, output } = require('proc-log') +// Ranks competing representations of the same physical package so the most logical one is reported. +// A top-level placement (e.g. node_modules/) beats the canonical store node, which beats an internal store symlink. +const locationRank = (node) => { + if (!node.location.includes('node_modules/.store/')) { + return 2 + } + return node.isLink ? 0 : 1 +} + class QuerySelectorItem { constructor (node) { // all enumerable properties from the target @@ -9,8 +18,11 @@ class QuerySelectorItem { // append extra info this.pkgid = node.target.pkgid - this.location = node.target.location - this.path = node.target.path + // For a dep symlinked into the isolated store, report the logical link location (node_modules/) rather than the .store backing path. + // Workspaces and regular nodes keep the target location (e.g. packages/). + const logical = node.target.isInStore ? node : node.target + this.location = logical.location + this.path = logical.path this.realpath = node.target.realpath this.resolved = node.target.resolved this.from = [] @@ -33,7 +45,7 @@ class QuerySelectorItem { class Query extends BaseCommand { #response = [] // response is the query response - #seen = new Set() // paths we've seen so we can keep response deduped + #seen = new Map() // physical location -> index in #response, to keep response deduped static description = 'Retrieve a filtered list of packages' static name = 'query' @@ -127,12 +139,22 @@ class Query extends BaseCommand { async #queryTree (tree, arg) { const items = await tree.querySelectorAll(arg, this.npm.flatOptions) for (const node of items) { + // Dedup by the target's physical location so multiple logical links to the same store node collapse to one result. const { location } = node.target - if (!location || !this.#seen.has(location)) { - const item = new QuerySelectorItem(node) - this.#response.push(item) - if (location) { - this.#seen.add(item.location) + if (!location) { + this.#response.push(new QuerySelectorItem(node)) + continue + } + const seen = this.#seen.get(location) + if (seen === undefined) { + this.#seen.set(location, { index: this.#response.length, rank: locationRank(node) }) + this.#response.push(new QuerySelectorItem(node)) + } else { + // Replace the stored representation only with a more logical one for the same physical package. + const rank = locationRank(node) + if (rank > seen.rank) { + this.#response[seen.index] = new QuerySelectorItem(node) + seen.rank = rank } } } diff --git a/test/lib/commands/query.js b/test/lib/commands/query.js index 40dadc8885836..de3acd0ff8b98 100644 --- a/test/lib/commands/query.js +++ b/test/lib/commands/query.js @@ -160,6 +160,61 @@ t.test('linked node', async t => { t.matchSnapshot(joinedOutput(), 'should return linked node res') }) +t.test('linked strategy reports logical location, not store backing path', async t => { + /* linked layout: nopt (direct) symlinked to its store key, abbrev a transitive dep of nopt */ + const linkedDir = { + prefixDir: { + node_modules: { + nopt: t.fixture('symlink', '.store/nopt-hash/node_modules/nopt'), + '.store': { + 'nopt-hash': { + node_modules: { + nopt: { + 'package.json': JSON.stringify({ + name: 'nopt', + version: '7.2.1', + dependencies: { abbrev: '^2.0.0' }, + }), + }, + abbrev: t.fixture('symlink', '../../abbrev-hash/node_modules/abbrev'), + }, + }, + 'abbrev-hash': { + node_modules: { + abbrev: { + 'package.json': JSON.stringify({ name: 'abbrev', version: '2.0.0' }), + }, + }, + }, + }, + }, + 'package.json': JSON.stringify({ + name: 'project', + version: '1.0.0', + dependencies: { nopt: '^7.0.0' }, + }), + }, + } + + await t.test(':root > * reports the logical link location', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, linkedDir) + await npm.exec('query', [':root > *']) + const res = JSON.parse(joinedOutput()) + t.equal(res.length, 1, 'returns a single result, not the store node duplicate') + t.equal(res[0].location, 'node_modules/nopt', 'reports the logical location') + t.match(res[0].realpath, /\.store/, 'realpath still resolves to the store') + }) + + await t.test(':root * keeps direct deps logical and transitive deps canonical', async t => { + const { npm, joinedOutput } = await loadMockNpm(t, linkedDir) + await npm.exec('query', [':root *']) + const byName = Object.fromEntries(JSON.parse(joinedOutput()).map(n => [n.name, n.location])) + t.equal(byName.nopt, 'node_modules/nopt', 'direct dep keeps its logical location') + t.equal(byName.abbrev, 'node_modules/.store/abbrev-hash/node_modules/abbrev', + 'transitive dep reports its canonical store key, not a consumer symlink') + }) +}) + t.test('global', async t => { const { npm, joinedOutput } = await loadMockNpm(t, { config: { diff --git a/workspaces/arborist/lib/query-selector-all.js b/workspaces/arborist/lib/query-selector-all.js index 3a6c15139d40b..05faa551f76ee 100644 --- a/workspaces/arborist/lib/query-selector-all.js +++ b/workspaces/arborist/lib/query-selector-all.js @@ -798,7 +798,8 @@ const hasParent = (node, compareNodes) => { // Only match if the node has a link whose parent is the compareNode. Without this check, nodes deep in the store (linked strategy) would incorrectly match as children of root via their fsParent chain. if (node.isTop && (node.resolveParent === compareNode)) { for (const link of node.linksIn) { - if (link.parent === compareNode) { + // A store-backing node (linked strategy) is reached through its logical Link, which matches via edgesIn below, so the store node itself is never a direct child. + if (link.parent === compareNode && !node.isInStore) { return true } } diff --git a/workspaces/arborist/test/query-selector-all.js b/workspaces/arborist/test/query-selector-all.js index 3886efc334d0c..7fc6d97b71951 100644 --- a/workspaces/arborist/test/query-selector-all.js +++ b/workspaces/arborist/test/query-selector-all.js @@ -1144,6 +1144,13 @@ t.test('linked strategy: :root > * excludes transitive deps and store nodes', as 'nopt@7.2.1', ], ':root > * should only return direct dependencies, not transitive deps or store nodes') + // :root > * must return the logical Links, not the store backing nodes + const rawChildren = await q(tree, ':root > *') + t.same(rawChildren.map(n => n.location).sort(), [ + 'node_modules/ini', + 'node_modules/nopt', + ], ':root > * returns the logical link locations, not .store backing paths') + const rootDescendants = await querySelectorAll(tree, ':root *') t.same(rootDescendants, [ 'abbrev@2.0.0',