Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(arborist): Allow downgrades to hoisted version dedupe workspace i… #8168

Merged
merged 1 commit into from
Mar 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
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
7 changes: 6 additions & 1 deletion workspaces/arborist/lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ class Node {
// return true if it's safe to remove this node, because anything that
// is depending on it would be fine with the thing that they would resolve
// to if it was removed, or nothing is depending on it in the first place.
canDedupe (preferDedupe = false) {
canDedupe (preferDedupe = false, explicitRequest = false) {
// not allowed to mess with shrinkwraps or bundles
if (this.inDepBundle || this.inShrinkwrap) {
return false
Expand Down Expand Up @@ -1117,6 +1117,11 @@ class Node {
return true
}

// if the other version was an explicit request, then prefer to take the other version
if (explicitRequest) {
return true
}

return false
}

Expand Down
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/place-dep.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ class PlaceDep {
// is another satisfying node further up the tree, and if so, dedupes.
// Even in installStategy is nested, we do this amount of deduplication.
pruneDedupable (node, descend = true) {
if (node.canDedupe(this.preferDedupe)) {
if (node.canDedupe(this.preferDedupe, this.explicitRequest)) {
// gather up all deps that have no valid edges in from outside
// the dep set, except for this node we're deduping, so that we
// also prune deps that would be made extraneous.
Expand Down
80 changes: 80 additions & 0 deletions workspaces/arborist/test/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -3427,3 +3427,83 @@ t.test('install stategy linked', async (t) => {
t.ok(abbrev.isSymbolicLink(), 'abbrev got installed')
})
})

t.test('workspace installs retain existing versions with newer package specs', async t => {
const path = t.testdir({
'package.json': JSON.stringify({
workspaces: [
'packages/*',
],
overrides: {
'doesnt-matter-can-be-anything': '1.2.3',
},
}),
packages: {
'my-cool-package': {
'package.json': JSON.stringify({}),
},
'another-cool-package': {
'package.json': JSON.stringify({}),
},
},
})

createRegistry(t, true)

// Step 1: Install [email protected] in my-cool-package
await reify(path, {
add: ['[email protected]'],
// setting savePrefix to '' is exactly what the --save-exact flag does in definitions.js
savePrefix: '',
workspaces: ['my-cool-package'],
})

// Verify hoisted installation
const rootNodeModules = resolve(path, 'node_modules/abbrev/package.json')
t.ok(fs.existsSync(rootNodeModules), 'abbrev should be hoisted to root node_modules')

const hoistedPkg = JSON.parse(fs.readFileSync(rootNodeModules, 'utf8'))
t.equal(hoistedPkg.version, '1.0.4', 'hoisted version should be 1.0.4')

// Check my-cool-package package.json
const myPackageJson = JSON.parse(fs.readFileSync(
resolve(path, 'packages/my-cool-package/package.json'), 'utf8'))
t.same(myPackageJson.dependencies, { abbrev: '1.0.4' },
'my-cool-package should have [email protected] in dependencies')

// Step 2: Install [email protected] in another-cool-package
await reify(path, {
add: ['[email protected]'],
savePrefix: '',
workspaces: ['another-cool-package'],
})

// Verify un-hoisted installation
const anotherNodeModules = resolve(path, 'packages/another-cool-package/node_modules/abbrev/package.json')
t.ok(fs.existsSync(anotherNodeModules), '[email protected] should be installed in another-cool-package/node_modules')

const unhoistedPkg = JSON.parse(fs.readFileSync(anotherNodeModules, 'utf8'))
t.equal(unhoistedPkg.version, '1.1.1', 'unhoisted version should be 1.1.1')

// Check another-cool-package package.json
const anotherPackageJson = JSON.parse(fs.readFileSync(
resolve(path, 'packages/another-cool-package/package.json'), 'utf8'))
t.same(anotherPackageJson.dependencies, { abbrev: '1.1.1' },
'another-cool-package should have [email protected] in dependencies')

// Step 3: Install [email protected] in another-cool-package
await reify(path, {
add: ['[email protected]'],
savePrefix: '',
workspaces: ['another-cool-package'],
})

t.ok(fs.existsSync(rootNodeModules), '[email protected] should still be hoisted to root node_modules')
t.notOk(fs.existsSync(anotherNodeModules), '[email protected] should be removed from another-cool-package/node_modules')

// Check another-cool-package package.json - should now be updated to 1.0.4
const updatedPackageJson = JSON.parse(fs.readFileSync(
resolve(path, 'packages/another-cool-package/package.json'), 'utf8'))
t.same(updatedPackageJson.dependencies, { abbrev: '1.0.4' },
'another-cool-package package.json should be updated to [email protected]')
})
37 changes: 37 additions & 0 deletions workspaces/arborist/test/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -2522,6 +2522,43 @@ t.test('canDedupe()', t => {
t.end()
})

t.test('canDedupe returns true when explicitRequest is true regardless of other conditions', t => {
// Create a minimal tree with a valid resolveParent
const root = new Node({
pkg: { name: 'root', version: '1.0.0' },
path: '/root',
realpath: '/root',
})

// Create a duplicate candidate node in the tree
const duplicate = new Node({
pkg: { name: 'dup', version: '1.0.0' },
parent: root,
})

// Create a node with the same name but a higher version so that normally dedupe would not occur
const node = new Node({
pkg: { name: 'dup', version: '2.0.0' },
parent: duplicate,
})

// Manually add an incoming edge so that node.edgesIn is non-empty
node.edgesIn.add({
from: duplicate,
satisfiedBy () {
return true
},
})

const preferDedupe = false
let explicitRequest = false
t.notOk(node.canDedupe(preferDedupe, explicitRequest), 'without explicit request, dedupe is not allowed')

explicitRequest = true
t.ok(node.canDedupe(preferDedupe, explicitRequest), 'explicit request forces dedupe to return true')
t.end()
})

t.test('packageName getter', t => {
const node = new Node({
pkg: { name: 'foo' },
Expand Down
Loading