fix: don't override bins of direct deps with bins of hoisted ones (#3795)

close #3784

.changeset/clean-teachers-eat.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/headless": patch
+"supi": patch
+---
+
+Do not override the bins of direct dependencies with the bins of hoisted dependencies.

.changeset/good-poems-invite.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/link-bins": minor
+---
+
+`linkBins()` accepts the project manifest and prioritizes the bins of its direct dependencies over the bin files of the hoisted dependencies.

package.json

@@ -31,7 +31,7 @@
     "@commitlint/prompt-cli": "^12.1.1",
     "@pnpm/eslint-config": "workspace:*",
     "@pnpm/meta-updater": "0.0.6",
-    "@pnpm/registry-mock": "^2.7.1",
+    "@pnpm/registry-mock": "^2.8.0",
     "@pnpm/tsconfig": "workspace:*",
     "@types/jest": "^26.0.23",
     "@types/node": "^14.14.33",

packages/headless/src/index.ts

@@ -441,8 +441,9 @@ export default async (opts: HeadlessOptions) => {
 }
 
 async function linkBinsOfImporter (
-  { modulesDir, binsDir, rootDir }: {
+  { manifest, modulesDir, binsDir, rootDir }: {
     binsDir: string
+    manifest: ProjectManifest
     modulesDir: string
     rootDir: string
   }
@@ -450,6 +451,7 @@ async function linkBinsOfImporter (
   const warn = (message: string) => logger.info({ message, prefix: rootDir })
   return linkBins(modulesDir, binsDir, {
     allowExoticManifests: true,
+    projectManifest: manifest,
     warn,
   })
 }

packages/link-bins/package.json

@@ -33,6 +33,7 @@
   "homepage": "https://github.com/pnpm/pnpm/blob/master/packages/link-bins#readme",
   "dependencies": {
     "@pnpm/error": "workspace:2.0.0",
+    "@pnpm/manifest-utils": "workspace:2.0.4",
     "@pnpm/package-bins": "workspace:5.0.5",
     "@pnpm/read-modules-dir": "workspace:3.0.1",
     "@pnpm/read-package-json": "workspace:5.0.4",

packages/link-bins/src/index.ts

@@ -2,11 +2,12 @@ import { promises as fs } from 'fs'
 import Module from 'module'
 import path from 'path'
 import PnpmError from '@pnpm/error'
+import { getAllDependenciesFromManifest } from '@pnpm/manifest-utils'
 import binify, { Command } from '@pnpm/package-bins'
 import readModulesDir from '@pnpm/read-modules-dir'
 import { fromDir as readPackageJsonFromDir } from '@pnpm/read-package-json'
 import { safeReadProjectManifestOnly } from '@pnpm/read-project-manifest'
-import { DependencyManifest } from '@pnpm/types'
+import { DependencyManifest, ProjectManifest } from '@pnpm/types'
 import cmdShim from '@zkochan/cmd-shim'
 import isSubdir from 'is-subdir'
 import isWindows from 'is-windows'
@@ -33,6 +34,7 @@ export default async (
   opts: {
     allowExoticManifests?: boolean
     nodeExecPathByAlias?: Record<string, string>
+    projectManifest?: ProjectManifest
     warn: WarnFunction
   }
 ): Promise<string[]> => {
@@ -43,23 +45,38 @@ export default async (
     allowExoticManifests: false,
     ...opts,
   }
+  const directDependencies = opts.projectManifest == null
+    ? undefined
+    : new Set(Object.keys(getAllDependenciesFromManifest(opts.projectManifest)))
   const allCmds = unnest(
     (await Promise.all(
       allDeps
         .map((alias) => ({
           depDir: path.resolve(modulesDir, alias),
+          isDirectDependency: directDependencies?.has(alias),
           nodeExecPath: opts.nodeExecPathByAlias?.[alias],
         }))
         .filter(({ depDir }) => !isSubdir(depDir, binsDir)) // Don't link own bins
-        .map(({ depDir, nodeExecPath }) => {
+        .map(async ({ depDir, isDirectDependency, nodeExecPath }) => {
           const target = normalizePath(depDir)
-          return getPackageBins(pkgBinOpts, target, nodeExecPath)
+          const cmds = await getPackageBins(pkgBinOpts, target, nodeExecPath)
+          return cmds.map((cmd) => ({ ...cmd, isDirectDependency }))
         })
     ))
       .filter((cmds: Command[]) => cmds.length)
   )
 
-  return linkBins(allCmds, binsDir, opts)
+  const cmdsToLink = directDependencies != null ? preferDirectCmds(allCmds) : allCmds
+  return linkBins(cmdsToLink, binsDir, opts)
+}
+
+function preferDirectCmds (allCmds: Array<CommandInfo & { isDirectDependency?: boolean }>) {
+  const [directCmds, hoistedCmds] = partition((cmd) => cmd.isDirectDependency === true, allCmds)
+  const usedDirectCmds = new Set(directCmds.map((directCmd) => directCmd.name))
+  return [
+    ...directCmds,
+    ...hoistedCmds.filter(({ name }) => !usedDirectCmds.has(name)),
+  ]
 }
 
 export async function linkBinsOfPackages (

packages/link-bins/test/index.ts

@@ -218,6 +218,37 @@ test('linkBinsOfPackages() resolves conflicts. Prefer packages that use their na
   }
 })
 
+test('linkBins() resolves conflicts. Prefer packages are direct dependencies', async () => {
+  const binTarget = tempy.directory()
+  const warn = jest.fn()
+
+  await linkBins(path.join(binNameConflictsFixture, 'node_modules'), binTarget, {
+    projectManifest: {
+      dependencies: {
+        foo: '1.0.0',
+      },
+    },
+    warn,
+  })
+
+  expect(warn).not.toHaveBeenCalled() // With(`Cannot link binary 'bar' of 'foo' to '${binTarget}': binary of 'bar' is already linked`, 'BINARIES_CONFLICT')
+  expect(await fs.readdir(binTarget)).toEqual(getExpectedBins(['bar', 'foofoo']))
+
+  {
+    const binLocation = path.join(binTarget, 'bar')
+    expect(await exists(binLocation)).toBe(true)
+    const content = await fs.readFile(binLocation, 'utf8')
+    expect(content).toMatch('node_modules/foo/index.js')
+  }
+
+  {
+    const binLocation = path.join(binTarget, 'foofoo')
+    expect(await exists(binLocation)).toBe(true)
+    const content = await fs.readFile(binLocation, 'utf8')
+    expect(content).toMatch('node_modules/foo/index.js')
+  }
+})
+
 test('linkBins() would throw error if package has no name field', async () => {
   const binTarget = tempy.directory()
   const warn = jest.fn()
@@ -267,4 +298,4 @@ test('linkBins() links commands from bin directory with a subdirectory', async (
   await linkBins(path.join(fixtures, 'bin-dir'), binTarget, { warn: () => {} })
 
   expect(await fs.readdir(binTarget)).toEqual(getExpectedBins(['index.js']))
-})
+})

packages/link-bins/tsconfig.json

@@ -12,6 +12,9 @@
     {
       "path": "../error"
     },
+    {
+      "path": "../manifest-utils"
+    },
     {
       "path": "../package-bins"
     },

packages/supi/src/install/index.ts

@@ -901,6 +901,7 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => {
           }, {})
         linkedPackages = await linkBins(project.modulesDir, project.binsDir, {
           allowExoticManifests: true,
+          projectManifest: project.manifest,
           nodeExecPathByAlias,
           warn: binWarn.bind(null, project.rootDir),
         })

packages/supi/test/install/hoist.ts

@@ -526,3 +526,23 @@ test('hoisting should not create a broken symlink to a skipped optional dependen
   await project.hasNot('dep-of-optional-pkg')
   expect(await rootModules.readCurrentLockfile()).toStrictEqual(await rootModules.readLockfile())
 })
+
+test('the hoisted packages should not override the bin files of the direct dependencies', async () => {
+  prepareEmpty()
+
+  const manifest = await addDependenciesToPackage({}, ['hello-world-js-bin-parent'], await testDefaults({ fastUnpack: false, publicHoistPattern: '*' }))
+
+  {
+    const cmd = await fs.promises.readFile('node_modules/.bin/hello-world-js-bin', 'utf-8')
+    expect(cmd).toContain('/hello-world-js-bin-parent/')
+  }
+
+  await rimraf('node_modules')
+
+  await install(manifest, await testDefaults({ fastUnpack: false, frozenLockfile: true, publicHoistPattern: '*' }))
+
+  {
+    const cmd = await fs.promises.readFile('node_modules/.bin/hello-world-js-bin', 'utf-8')
+    expect(cmd).toContain('/hello-world-js-bin-parent/')
+  }
+})

pnpm-lock.yaml

@@ -33,7 +33,7 @@ importers:
       '@commitlint/prompt-cli': ^12.1.1
       '@pnpm/eslint-config': workspace:*
       '@pnpm/meta-updater': 0.0.6
-      '@pnpm/registry-mock': ^2.7.1
+      '@pnpm/registry-mock': ^2.8.0
       '@pnpm/tsconfig': workspace:*
       '@types/jest': ^26.0.23
       '@types/node': ^14.14.33
@@ -61,7 +61,7 @@ importers:
       '@commitlint/prompt-cli': 12.1.4
       '@pnpm/eslint-config': link:utils/eslint-config
       '@pnpm/meta-updater': 0.0.6
-      '@pnpm/registry-mock': 2.7.1
+      '@pnpm/registry-mock': 2.8.0
       '@pnpm/tsconfig': link:utils/tsconfig
       '@types/jest': 26.0.24
       '@types/node': 14.17.19
@@ -970,6 +970,7 @@ importers:
     specifiers:
       '@pnpm/error': workspace:2.0.0
       '@pnpm/link-bins': 'link:'
+      '@pnpm/manifest-utils': workspace:2.0.4
       '@pnpm/package-bins': workspace:5.0.5
       '@pnpm/read-modules-dir': workspace:3.0.1
       '@pnpm/read-package-json': workspace:5.0.4
@@ -991,6 +992,7 @@ importers:
       tempy: ^1.0.0
     dependencies:
       '@pnpm/error': link:../error
+      '@pnpm/manifest-utils': link:../manifest-utils
       '@pnpm/package-bins': link:../package-bins
       '@pnpm/read-modules-dir': link:../read-modules-dir
       '@pnpm/read-package-json': link:../read-package-json
@@ -4644,8 +4646,8 @@ packages:
       strip-bom: 4.0.0
     dev: true
 
-  /@pnpm/registry-mock/2.7.1:
-    resolution: {integrity: sha512-rPZldydDKWuXjQ4PvRRmlXNTvMypY8ByH4/YbVKSVnmd0zou/LGa77qUcssWoL/oQguN6o7jN7iwVEi54Gbx3A==}
+  /@pnpm/registry-mock/2.8.0:
+    resolution: {integrity: sha512-Pfvl8wbcetA+m/95LwX3q0/Q+DupL6gHBc9yZJeLDlqZ6Q65B9PykeUR+xRw1qipXvBTr8OTJ7joQKkak0VqCQ==}
     engines: {node: '>=10.13'}
     hasBin: true
     dependencies: