Skip to content

Commit 8f13beb

Browse files
github-actions[bot]manzoorwanijk
github-actions[bot]
and
manzoorwanijk
authored
fix(query): report logical dep location under linked strategy (#9664)
Backport of #9656 to `release/v11`. Co-authored-by: Manzoor Wani <manzoorwani.jk@gmail.com>
1 parent 168ba30 commit 8f13beb

4 files changed

Lines changed: 94 additions & 9 deletions

File tree

‎lib/commands/query.js‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,27 @@ const { resolve } = require('node:path')
22
const BaseCommand = require('../base-cmd.js')
33
const { log, output } = require('proc-log')
44

5+
// Ranks competing representations of the same physical package so the most logical one is reported.
6+
// A top-level placement (e.g. node_modules/<pkg>) beats the canonical store node, which beats an internal store symlink.
7+
const locationRank = (node) => {
8+
if (!node.location.includes('node_modules/.store/')) {
9+
return 2
10+
}
11+
return node.isLink ? 0 : 1
12+
}
13+
514
class QuerySelectorItem {
615
constructor (node) {
716
// all enumerable properties from the target
817
Object.assign(this, node.target.package)
918

1019
// append extra info
1120
this.pkgid = node.target.pkgid
12-
this.location = node.target.location
13-
this.path = node.target.path
21+
// For a dep symlinked into the isolated store, report the logical link location (node_modules/<pkg>) rather than the .store backing path.
22+
// Workspaces and regular nodes keep the target location (e.g. packages/<ws>).
23+
const logical = node.target.isInStore ? node : node.target
24+
this.location = logical.location
25+
this.path = logical.path
1426
this.realpath = node.target.realpath
1527
this.resolved = node.target.resolved
1628
this.from = []
@@ -33,7 +45,7 @@ class QuerySelectorItem {
3345

3446
class Query extends BaseCommand {
3547
#response = [] // response is the query response
36-
#seen = new Set() // paths we've seen so we can keep response deduped
48+
#seen = new Map() // physical location -> index in #response, to keep response deduped
3749

3850
static description = 'Retrieve a filtered list of packages'
3951
static name = 'query'
@@ -127,12 +139,22 @@ class Query extends BaseCommand {
127139
async #queryTree (tree, arg) {
128140
const items = await tree.querySelectorAll(arg, this.npm.flatOptions)
129141
for (const node of items) {
142+
// Dedup by the target's physical location so multiple logical links to the same store node collapse to one result.
130143
const { location } = node.target
131-
if (!location || !this.#seen.has(location)) {
132-
const item = new QuerySelectorItem(node)
133-
this.#response.push(item)
134-
if (location) {
135-
this.#seen.add(item.location)
144+
if (!location) {
145+
this.#response.push(new QuerySelectorItem(node))
146+
continue
147+
}
148+
const seen = this.#seen.get(location)
149+
if (seen === undefined) {
150+
this.#seen.set(location, { index: this.#response.length, rank: locationRank(node) })
151+
this.#response.push(new QuerySelectorItem(node))
152+
} else {
153+
// Replace the stored representation only with a more logical one for the same physical package.
154+
const rank = locationRank(node)
155+
if (rank > seen.rank) {
156+
this.#response[seen.index] = new QuerySelectorItem(node)
157+
seen.rank = rank
136158
}
137159
}
138160
}

‎test/lib/commands/query.js‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,61 @@ t.test('linked node', async t => {
160160
t.matchSnapshot(joinedOutput(), 'should return linked node res')
161161
})
162162

163+
t.test('linked strategy reports logical location, not store backing path', async t => {
164+
/* linked layout: nopt (direct) symlinked to its store key, abbrev a transitive dep of nopt */
165+
const linkedDir = {
166+
prefixDir: {
167+
node_modules: {
168+
nopt: t.fixture('symlink', '.store/nopt-hash/node_modules/nopt'),
169+
'.store': {
170+
'nopt-hash': {
171+
node_modules: {
172+
nopt: {
173+
'package.json': JSON.stringify({
174+
name: 'nopt',
175+
version: '7.2.1',
176+
dependencies: { abbrev: '^2.0.0' },
177+
}),
178+
},
179+
abbrev: t.fixture('symlink', '../../abbrev-hash/node_modules/abbrev'),
180+
},
181+
},
182+
'abbrev-hash': {
183+
node_modules: {
184+
abbrev: {
185+
'package.json': JSON.stringify({ name: 'abbrev', version: '2.0.0' }),
186+
},
187+
},
188+
},
189+
},
190+
},
191+
'package.json': JSON.stringify({
192+
name: 'project',
193+
version: '1.0.0',
194+
dependencies: { nopt: '^7.0.0' },
195+
}),
196+
},
197+
}
198+
199+
await t.test(':root > * reports the logical link location', async t => {
200+
const { npm, joinedOutput } = await loadMockNpm(t, linkedDir)
201+
await npm.exec('query', [':root > *'])
202+
const res = JSON.parse(joinedOutput())
203+
t.equal(res.length, 1, 'returns a single result, not the store node duplicate')
204+
t.equal(res[0].location, 'node_modules/nopt', 'reports the logical location')
205+
t.match(res[0].realpath, /\.store/, 'realpath still resolves to the store')
206+
})
207+
208+
await t.test(':root * keeps direct deps logical and transitive deps canonical', async t => {
209+
const { npm, joinedOutput } = await loadMockNpm(t, linkedDir)
210+
await npm.exec('query', [':root *'])
211+
const byName = Object.fromEntries(JSON.parse(joinedOutput()).map(n => [n.name, n.location]))
212+
t.equal(byName.nopt, 'node_modules/nopt', 'direct dep keeps its logical location')
213+
t.equal(byName.abbrev, 'node_modules/.store/abbrev-hash/node_modules/abbrev',
214+
'transitive dep reports its canonical store key, not a consumer symlink')
215+
})
216+
})
217+
163218
t.test('global', async t => {
164219
const { npm, joinedOutput } = await loadMockNpm(t, {
165220
config: {

‎workspaces/arborist/lib/query-selector-all.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -798,7 +798,8 @@ const hasParent = (node, compareNodes) => {
798798
// 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.
799799
if (node.isTop && (node.resolveParent === compareNode)) {
800800
for (const link of node.linksIn) {
801-
if (link.parent === compareNode) {
801+
// 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.
802+
if (link.parent === compareNode && !node.isInStore) {
802803
return true
803804
}
804805
}

‎workspaces/arborist/test/query-selector-all.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,6 +1144,13 @@ t.test('linked strategy: :root > * excludes transitive deps and store nodes', as
11441144
'nopt@7.2.1',
11451145
], ':root > * should only return direct dependencies, not transitive deps or store nodes')
11461146

1147+
// :root > * must return the logical Links, not the store backing nodes
1148+
const rawChildren = await q(tree, ':root > *')
1149+
t.same(rawChildren.map(n => n.location).sort(), [
1150+
'node_modules/ini',
1151+
'node_modules/nopt',
1152+
], ':root > * returns the logical link locations, not .store backing paths')
1153+
11471154
const rootDescendants = await querySelectorAll(tree, ':root *')
11481155
t.same(rootDescendants, [
11491156
'abbrev@2.0.0',

0 commit comments

Comments
 (0)