Skip to content

Commit 8b7bb12

Browse files
authored
fix(arborist): Allow downgrades to hoisted version dedupe workspace i… (#8168)
fixes #7028 The crux of the issue as that when the downgrade was attempting to dedupe, there was nothing in the `canDedupe` logic that said it was okay to take the other version if it was an explicit request. It would see the 0.0.2 in the root, the 0.0.3 in the workspace, and give up, leaving them both as they were. The proposed change adds a new parameter `explicitRequest` to the `canDedupe` method with a default value of false. This parameter enables dedupe behavior when a package version was explicitly requested by the user. Adding the `explicitRequest` parameter introduces a new condition that allows deduping when: - A user has explicitly requested a specific package version via commands like `npm install package@version` - None of the other deduping criteria are met - The current version isn't already the result of an override I believe this was just an edge case that wasn't handled in the dedupe logic, and this change should fix it.
1 parent 1642556 commit 8b7bb12

File tree

4 files changed

+124
-2
lines changed

4 files changed

+124
-2
lines changed

workspaces/arborist/lib/node.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -1074,7 +1074,7 @@ class Node {
10741074
// return true if it's safe to remove this node, because anything that
10751075
// is depending on it would be fine with the thing that they would resolve
10761076
// to if it was removed, or nothing is depending on it in the first place.
1077-
canDedupe (preferDedupe = false) {
1077+
canDedupe (preferDedupe = false, explicitRequest = false) {
10781078
// not allowed to mess with shrinkwraps or bundles
10791079
if (this.inDepBundle || this.inShrinkwrap) {
10801080
return false
@@ -1117,6 +1117,11 @@ class Node {
11171117
return true
11181118
}
11191119

1120+
// if the other version was an explicit request, then prefer to take the other version
1121+
if (explicitRequest) {
1122+
return true
1123+
}
1124+
11201125
return false
11211126
}
11221127

workspaces/arborist/lib/place-dep.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ class PlaceDep {
423423
// is another satisfying node further up the tree, and if so, dedupes.
424424
// Even in installStategy is nested, we do this amount of deduplication.
425425
pruneDedupable (node, descend = true) {
426-
if (node.canDedupe(this.preferDedupe)) {
426+
if (node.canDedupe(this.preferDedupe, this.explicitRequest)) {
427427
// gather up all deps that have no valid edges in from outside
428428
// the dep set, except for this node we're deduping, so that we
429429
// also prune deps that would be made extraneous.

workspaces/arborist/test/arborist/reify.js

+80
Original file line numberDiff line numberDiff line change
@@ -3427,3 +3427,83 @@ t.test('install stategy linked', async (t) => {
34273427
t.ok(abbrev.isSymbolicLink(), 'abbrev got installed')
34283428
})
34293429
})
3430+
3431+
t.test('workspace installs retain existing versions with newer package specs', async t => {
3432+
const path = t.testdir({
3433+
'package.json': JSON.stringify({
3434+
workspaces: [
3435+
'packages/*',
3436+
],
3437+
overrides: {
3438+
'doesnt-matter-can-be-anything': '1.2.3',
3439+
},
3440+
}),
3441+
packages: {
3442+
'my-cool-package': {
3443+
'package.json': JSON.stringify({}),
3444+
},
3445+
'another-cool-package': {
3446+
'package.json': JSON.stringify({}),
3447+
},
3448+
},
3449+
})
3450+
3451+
createRegistry(t, true)
3452+
3453+
// Step 1: Install abbrev@1.0.4 in my-cool-package
3454+
await reify(path, {
3455+
add: ['abbrev@1.0.4'],
3456+
// setting savePrefix to '' is exactly what the --save-exact flag does in definitions.js
3457+
savePrefix: '',
3458+
workspaces: ['my-cool-package'],
3459+
})
3460+
3461+
// Verify hoisted installation
3462+
const rootNodeModules = resolve(path, 'node_modules/abbrev/package.json')
3463+
t.ok(fs.existsSync(rootNodeModules), 'abbrev should be hoisted to root node_modules')
3464+
3465+
const hoistedPkg = JSON.parse(fs.readFileSync(rootNodeModules, 'utf8'))
3466+
t.equal(hoistedPkg.version, '1.0.4', 'hoisted version should be 1.0.4')
3467+
3468+
// Check my-cool-package package.json
3469+
const myPackageJson = JSON.parse(fs.readFileSync(
3470+
resolve(path, 'packages/my-cool-package/package.json'), 'utf8'))
3471+
t.same(myPackageJson.dependencies, { abbrev: '1.0.4' },
3472+
'my-cool-package should have abbrev@1.0.4 in dependencies')
3473+
3474+
// Step 2: Install abbrev@1.1.1 in another-cool-package
3475+
await reify(path, {
3476+
add: ['abbrev@1.1.1'],
3477+
savePrefix: '',
3478+
workspaces: ['another-cool-package'],
3479+
})
3480+
3481+
// Verify un-hoisted installation
3482+
const anotherNodeModules = resolve(path, 'packages/another-cool-package/node_modules/abbrev/package.json')
3483+
t.ok(fs.existsSync(anotherNodeModules), 'abbrev@1.1.1 should be installed in another-cool-package/node_modules')
3484+
3485+
const unhoistedPkg = JSON.parse(fs.readFileSync(anotherNodeModules, 'utf8'))
3486+
t.equal(unhoistedPkg.version, '1.1.1', 'unhoisted version should be 1.1.1')
3487+
3488+
// Check another-cool-package package.json
3489+
const anotherPackageJson = JSON.parse(fs.readFileSync(
3490+
resolve(path, 'packages/another-cool-package/package.json'), 'utf8'))
3491+
t.same(anotherPackageJson.dependencies, { abbrev: '1.1.1' },
3492+
'another-cool-package should have abbrev@1.1.1 in dependencies')
3493+
3494+
// Step 3: Install abbrev@1.0.4 in another-cool-package
3495+
await reify(path, {
3496+
add: ['abbrev@1.0.4'],
3497+
savePrefix: '',
3498+
workspaces: ['another-cool-package'],
3499+
})
3500+
3501+
t.ok(fs.existsSync(rootNodeModules), 'abbrev@1.0.4 should still be hoisted to root node_modules')
3502+
t.notOk(fs.existsSync(anotherNodeModules), 'abbrev@1.1.1 should be removed from another-cool-package/node_modules')
3503+
3504+
// Check another-cool-package package.json - should now be updated to 1.0.4
3505+
const updatedPackageJson = JSON.parse(fs.readFileSync(
3506+
resolve(path, 'packages/another-cool-package/package.json'), 'utf8'))
3507+
t.same(updatedPackageJson.dependencies, { abbrev: '1.0.4' },
3508+
'another-cool-package package.json should be updated to abbrev@1.0.4')
3509+
})

workspaces/arborist/test/node.js

+37
Original file line numberDiff line numberDiff line change
@@ -2522,6 +2522,43 @@ t.test('canDedupe()', t => {
25222522
t.end()
25232523
})
25242524

2525+
t.test('canDedupe returns true when explicitRequest is true regardless of other conditions', t => {
2526+
// Create a minimal tree with a valid resolveParent
2527+
const root = new Node({
2528+
pkg: { name: 'root', version: '1.0.0' },
2529+
path: '/root',
2530+
realpath: '/root',
2531+
})
2532+
2533+
// Create a duplicate candidate node in the tree
2534+
const duplicate = new Node({
2535+
pkg: { name: 'dup', version: '1.0.0' },
2536+
parent: root,
2537+
})
2538+
2539+
// Create a node with the same name but a higher version so that normally dedupe would not occur
2540+
const node = new Node({
2541+
pkg: { name: 'dup', version: '2.0.0' },
2542+
parent: duplicate,
2543+
})
2544+
2545+
// Manually add an incoming edge so that node.edgesIn is non-empty
2546+
node.edgesIn.add({
2547+
from: duplicate,
2548+
satisfiedBy () {
2549+
return true
2550+
},
2551+
})
2552+
2553+
const preferDedupe = false
2554+
let explicitRequest = false
2555+
t.notOk(node.canDedupe(preferDedupe, explicitRequest), 'without explicit request, dedupe is not allowed')
2556+
2557+
explicitRequest = true
2558+
t.ok(node.canDedupe(preferDedupe, explicitRequest), 'explicit request forces dedupe to return true')
2559+
t.end()
2560+
})
2561+
25252562
t.test('packageName getter', t => {
25262563
const node = new Node({
25272564
pkg: { name: 'foo' },

0 commit comments

Comments
 (0)