Fix transient symlinks overriding direct ones v2

__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<void> => {
+  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<void> => {

__tests__/fixtures/install/install-bin-links-conflicting-names/a-dep/bin

@@ -0,0 +1,2 @@
+#!/usr/bin/env node
+console.log('direct a-dep');

__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');

__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"
+  }
+}

__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"
+  }
+}

__tests__/fixtures/install/install-bin-links-conflicting-names/f-dep/bin

@@ -0,0 +1,2 @@
+#!/usr/bin/env node
+console.log('direct f-dep');

__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');

__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"
+  }
+}

__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"
+  }
+}

__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"
+  }
+}

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() {