diff --git a/__tests__/commands/install/bin-links.js b/__tests__/commands/install/bin-links.js index 3e78a5c4ab..59208a058d 100644 --- a/__tests__/commands/install/bin-links.js +++ b/__tests__/commands/install/bin-links.js @@ -145,6 +145,17 @@ test('first dep is installed when same level and reference count and one is a de }); }); +// Scenario: Transitive dependency having bin link with a name that's conflicting with that of a direct dependency. +// Behavior: a-dep and b-dep is linked in node_modules/.bin rather than c-dep and d-dep +test('direct dependency is linked when bin name conflicts with transitive dependency', (): Promise => { + return runInstall({binLinks: true}, 'install-bin-links-conflicting-names', async config => { + const stdout1 = await execCommand(config.cwd, ['node_modules', '.bin', 'binlink1'], []); + const stdout2 = await execCommand(config.cwd, ['node_modules', '.bin', 'binlink2'], []); + expect(stdout1[0]).toEqual('direct a-dep'); + expect(stdout2[0]).toEqual('direct f-dep'); + }); +}); + // fixes https://github.com/yarnpkg/yarn/issues/3535 // quite a heavy test, did not find a way to isolate test('Only top level (after hoisting) bin links should be linked', (): Promise => { diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/bin b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/bin new file mode 100755 index 0000000000..d993532d9a --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/bin @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('direct a-dep'); diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/bin b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/bin new file mode 100755 index 0000000000..3953becac0 --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/bin @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('transient c-dep'); diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/package.json b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/package.json new file mode 100644 index 0000000000..f0b4669749 --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/c-dep/package.json @@ -0,0 +1,7 @@ +{ + "name": "c-dep", + "version": "0.0.1", + "bin": { + "binlink2": "./bin" + } +} diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/package.json b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/package.json new file mode 100644 index 0000000000..6be8db4d09 --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/package.json @@ -0,0 +1,10 @@ +{ + "name": "a-dep", + "version": "0.0.1", + "bin": { + "binlink1": "./bin" + }, + "dependencies": { + "c-dep": "file:c-dep" + } +} diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/bin b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/bin new file mode 100755 index 0000000000..526ad3e3f2 --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/bin @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('direct f-dep'); diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/bin b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/bin new file mode 100755 index 0000000000..856f0d437a --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/bin @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log('transient d-dep'); diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/package.json b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/package.json new file mode 100644 index 0000000000..725f563fea --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/d-dep/package.json @@ -0,0 +1,7 @@ +{ + "name": "d-dep", + "version": "0.0.1", + "bin": { + "binlink1": "./bin" + } +} diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/package.json b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/package.json new file mode 100644 index 0000000000..16c62e620f --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/package.json @@ -0,0 +1,10 @@ +{ + "name": "b-dep", + "version": "0.0.1", + "bin": { + "binlink2": "./bin" + }, + "dependencies": { + "d-dep": "file:d-dep" + } +} diff --git a/__tests__/fixtures/install/install-bin-links-conflicting-names/package.json b/__tests__/fixtures/install/install-bin-links-conflicting-names/package.json new file mode 100644 index 0000000000..4862ab982b --- /dev/null +++ b/__tests__/fixtures/install/install-bin-links-conflicting-names/package.json @@ -0,0 +1,6 @@ +{ + "dependencies": { + "a-dep": "file:a-dep", + "f-dep": "file:f-dep" + } +} diff --git a/src/package-linker.js b/src/package-linker.js index e32fc2480c..31236c36e2 100644 --- a/src/package-linker.js +++ b/src/package-linker.js @@ -401,7 +401,7 @@ export default class PackageLinker { // create binary links if (this.config.binLinks) { - const topLevelDependencies = this.determineTopLevelBinLinks(flatTree); + const topLevelDependencies = this.determineTopLevelBinLinkOrder(flatTree); const tickBin = this.reporter.progress(flatTree.length + topLevelDependencies.length); // create links in transient dependencies @@ -420,7 +420,7 @@ export default class PackageLinker { // create links at top level for all dependencies. await promise.queue( topLevelDependencies, - async ([dest, pkg]) => { + async ([dest, {pkg}]) => { if (pkg._reference && pkg._reference.location && pkg.bin && Object.keys(pkg.bin).length) { const binLoc = path.join(this.config.lockfileFolder, this.config.getFolder(pkg)); await this.linkSelfDependencies(pkg, dest, binLoc); @@ -436,17 +436,32 @@ export default class PackageLinker { } } - determineTopLevelBinLinks(flatTree: HoistManifestTuples): Array<[string, Manifest]> { + determineTopLevelBinLinkOrder(flatTree: HoistManifestTuples): HoistManifestTuples { const linksToCreate = new Map(); - for (const [dest, {pkg, isDirectRequire}] of flatTree) { + for (const [dest, hoistManifest] of flatTree) { + const {pkg, isDirectRequire} = hoistManifest; const {name} = pkg; if (isDirectRequire || (this.topLevelBinLinking && !linksToCreate.has(name))) { - linksToCreate.set(name, [dest, pkg]); + linksToCreate.set(name, [dest, hoistManifest]); } } - return Array.from(linksToCreate.values()); + // Sort the array so that direct dependencies will be linked last. + // Bin links are overwritten if they already exist, so this will cause direct deps to take precedence. + // If someone finds this to be incorrect later, you could also consider sorting descending by + // `linkToCreate.level` which is the dependency tree depth. Direct deps will have level 0 and transitive + // deps will have level > 0. + const transientBins = []; + const topLevelBins = []; + for (const linkToCreate of Array.from(linksToCreate.values())) { + if (linkToCreate[1].isDirectRequire) { + topLevelBins.push(linkToCreate); + } else { + transientBins.push(linkToCreate); + } + } + return [...transientBins, ...topLevelBins]; } resolvePeerModules() {