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