Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -898,8 +898,18 @@ This is a one-time fix-up, please be patient...
// be forced to agree on a version of z.
const required = new Set([edge.from])
const parent = edge.peer ? virtualRoot : null
const dep = vrDep && vrDep.satisfies(edge) ? vrDep
: await this.#nodeFromEdge(edge, parent, null, required)
let dep = vrDep && vrDep.satisfies(edge) ? vrDep : null

// A peerOptional conflict can be resolved by finding an existing
// node in the tree that satisfies the edge, avoiding a registry
// fetch that may introduce an extraneous package.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// A peerOptional conflict can be resolved by finding an existing
// node in the tree that satisfies the edge, avoiding a registry
// fetch that may introduce an extraneous package.
// A peerOptional conflict can be resolved by finding an existing node in the tree that satisfies the edge, avoiding a registry fetch that may introduce an extraneous package.

we try to break only at natural boundaries for screen reader accessibility

// See npm/cli#9249.
if (!dep && edge.type === 'peerOptional') {
dep = this.#findHoistableNode(edge.from.resolveParent || edge.from, edge)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!dep && edge.type === 'peerOptional') {
dep = this.#findHoistableNode(edge.from.resolveParent || edge.from, edge)
}
// Skip the shortcut when the user has signaled an explicit re-fetch intent (npm update by name, explicit request, or audit fix), so we honor those signals rather than silently keeping the existing node.
const skipExistingShortcut = this[_updateNames].includes(edge.name)
|| this.#explicitRequests.has(edge)
|| (edge.to && this.auditReport?.isVulnerable(edge.to))
if (!dep && edge.type === 'peerOptional' && !skipExistingShortcut) {
dep = this.#findHoistableNode(edge.from.resolveParent || edge.from, edge)
}

I think if we add this check, we get to keep the dedupe behavior for normal installs while still letting npm update, npm audit fix, and explicit installs do their thing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only in problemEdges tho, wouldn't there be other chances earlier to apply requested updates?

Aside: I noticed there is a test that peerOptionals are installed when explicitly requested, and that still passes npm install a-peer-optional.

Tho I noticed that even before this change the test would fail on re-install, or when package-lock or node_modules was already present. That seems bad, npm's only respecting the explciit request when it's not building from a lock.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see those count as problems and land here.

if (!dep) {
dep = await this.#nodeFromEdge(edge, parent, null, required)
}

/* istanbul ignore next */
debug(() => {
Expand Down Expand Up @@ -1029,6 +1039,24 @@ This is a one-time fix-up, please be patient...
return this.#buildDepStep()
}

// BFS descendants of `root` for a node satisfying `edge`.
// Prefers nodes closer to root. Skips bundled nodes.
#findHoistableNode (root, edge) {
const queue = [...root.children.values()]
while (queue.length) {
const node = queue.shift()
if (node.name === edge.name
&& !node.inDepBundle
&& node.satisfies(edge)) {
return node
}
for (const child of node.children.values()) {
queue.push(child)
}
}
return null
}

// loads a node from an edge, and then loads its peer deps (and their peer deps, on down the line) into a virtual root parent.
async #nodeFromEdge (edge, parent_, secondEdge, required) {
// create a virtual root node with the same deps as the node that is requesting this one, so that we can get all the peer deps in a context where they're likely to be resolvable.
Expand Down
76 changes: 76 additions & 0 deletions workspaces/arborist/test/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -4482,6 +4482,82 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)'
t.ok(tree.children.get('util'), 'util is in the tree')
})

t.test('peerOptional prefers existing tree node over registry fetch (#9249)', async t => {
// Reproduction: ts-jest has peerOptional jest-util@"^29||^30".
// @types/jest@28 → expect@28 → jest-util@28 placed at root first.
// jest@29 → jest-util@29 nested (root slot taken by @28).
// ts-jest re-queued, peerOptional jest-util resolves to root @28 → INVALID.
// Without fix: #nodeFromEdge fetches jest-util@30 (latest ^29||^30), blocks @29.
// With fix: #findHoistableNode finds nested @29, PlaceDep hoists it to root.
const registry = createRegistry(t, false)

const jestPack = registry.packument({
name: 'jest',
version: '29.0.0',
dependencies: { 'jest-util': '^29.0.0' },
})
const jestManifest = registry.manifest({ name: 'jest', packuments: [jestPack] })
await registry.package({ manifest: jestManifest })

const tsJestPack = registry.packument({
name: 'ts-jest',
version: '29.0.0',
peerDependencies: { jest: '^29.0.0', 'jest-util': '^29.0.0 || ^30.0.0' },
peerDependenciesMeta: { 'jest-util': { optional: true } },
})
const tsJestManifest = registry.manifest({ name: 'ts-jest', packuments: [tsJestPack] })
await registry.package({ manifest: tsJestManifest })

const expectPack = registry.packument({
name: 'expect',
version: '28.0.0',
dependencies: { 'jest-util': '^28.0.0' },
})
const expectManifest = registry.manifest({ name: 'expect', packuments: [expectPack] })
await registry.package({ manifest: expectManifest })

const atTypesPack = registry.packument({
name: '@types/jest',
version: '28.0.0',
dependencies: { expect: '^28.0.0' },
})
const atTypesManifest = registry.manifest({ name: '@types/jest', packuments: [atTypesPack] })
await registry.package({ manifest: atTypesManifest })

// Only publish 28, 29, and 30.
const jestUtilPacks = registry.packuments(['28.0.0', '29.0.0', '30.0.0'], 'jest-util')
const jestUtilManifest = registry.manifest({ name: 'jest-util', packuments: jestUtilPacks })
await registry.package({ manifest: jestUtilManifest, times: 3 })

const path = t.testdir({
'package.json': JSON.stringify({
dependencies: {
jest: '^29.0.0',
'ts-jest': '^29.0.0',
'@types/jest': '^28.0.0',
},
}),
})

const arb = newArb(path)
const tree = await arb.buildIdealTree()

// jest-util@29 at root — found via #findHoistableNode, not fetched as @30
t.equal(tree.children.get('jest-util').version, '29.0.0',
'jest-util@29 hoisted to root from nested location')

// ts-jest's peerOptional resolved to @29 from the tree, not @30 from registry
const tsJest = tree.children.get('ts-jest')
const peerOptEdge = tsJest.edgesOut.get('jest-util')
t.equal(peerOptEdge.to.version, '29.0.0',
'ts-jest peerOptional jest-util resolved to @29')

// jest-util@28 nested under expect (incompatible with root @29)
const expectNode = [...tree.inventory.query('name', 'expect')][0]
t.equal(expectNode?.children?.get('jest-util')?.version, '28.0.0',
'jest-util@28 nested under expect')
})

t.test('overrides with bundledDependencies', async t => {
t.test('does not infinite loop with bundledDependencies and overrides', async t => {
// https://github.com/npm/cli/issues/9227
Expand Down
Loading