fix: dedupe direct deps after hoisting (#6286)

.changeset/tall-zebras-accept.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/core": patch
+"pnpm": patch
+---
+
+Dedupe direct dependencies after hoisting.

pkg-manager/core/src/install/link.ts

@@ -157,52 +157,6 @@ export async function linkPackages (
     stage: 'importing_done',
   })
 
-  if (opts.symlink) {
-    const projectsToLink = Object.fromEntries(await Promise.all(
-      projects.map(async ({ id, manifest, modulesDir, rootDir }) => {
-        const deps = opts.dependenciesByProjectId[id]
-        const importerFromLockfile = newCurrentLockfile.importers[id]
-        return [id, {
-          dir: rootDir,
-          modulesDir,
-          dependencies: await Promise.all([
-            ...Object.entries(deps)
-              .filter(([rootAlias]) => importerFromLockfile.specifiers[rootAlias])
-              .map(([rootAlias, depPath]) => ({ rootAlias, depGraphNode: depGraph[depPath] }))
-              .filter(({ depGraphNode }) => depGraphNode)
-              .map(async ({ rootAlias, depGraphNode }) => {
-                const isDev = Boolean(manifest.devDependencies?.[depGraphNode.name])
-                const isOptional = Boolean(manifest.optionalDependencies?.[depGraphNode.name])
-                return {
-                  alias: rootAlias,
-                  name: depGraphNode.name,
-                  version: depGraphNode.version,
-                  dir: depGraphNode.dir,
-                  id: depGraphNode.id,
-                  dependencyType: (isDev && 'dev' || isOptional && 'optional' || 'prod') as 'dev' | 'optional' | 'prod',
-                  latest: opts.outdatedDependencies[depGraphNode.id],
-                  isExternalLink: false,
-                }
-              }),
-            ...opts.linkedDependenciesByProjectId[id].map(async (linkedDependency) => {
-              const dir = resolvePath(rootDir, linkedDependency.resolution.directory)
-              return {
-                alias: linkedDependency.alias,
-                name: linkedDependency.name,
-                version: linkedDependency.version,
-                dir,
-                id: linkedDependency.resolution.directory,
-                dependencyType: (linkedDependency.dev && 'dev' || linkedDependency.optional && 'optional' || 'prod') as 'dev' | 'optional' | 'prod',
-                isExternalLink: true,
-              }
-            }),
-          ]),
-        }]
-      }))
-    )
-    await linkDirectDeps(projectsToLink, { dedupe: opts.dedupeDirectDeps })
-  }
-
   let currentLockfile: Lockfile
   const allImportersIncluded = equals(projectIds.sort(), Object.keys(opts.wantedLockfile.importers).sort())
   if (
@@ -269,6 +223,52 @@ export async function linkPackages (
     newHoistedDependencies = opts.hoistedDependencies
   }
 
+  if (opts.symlink) {
+    const projectsToLink = Object.fromEntries(await Promise.all(
+      projects.map(async ({ id, manifest, modulesDir, rootDir }) => {
+        const deps = opts.dependenciesByProjectId[id]
+        const importerFromLockfile = newCurrentLockfile.importers[id]
+        return [id, {
+          dir: rootDir,
+          modulesDir,
+          dependencies: await Promise.all([
+            ...Object.entries(deps)
+              .filter(([rootAlias]) => importerFromLockfile.specifiers[rootAlias])
+              .map(([rootAlias, depPath]) => ({ rootAlias, depGraphNode: depGraph[depPath] }))
+              .filter(({ depGraphNode }) => depGraphNode)
+              .map(async ({ rootAlias, depGraphNode }) => {
+                const isDev = Boolean(manifest.devDependencies?.[depGraphNode.name])
+                const isOptional = Boolean(manifest.optionalDependencies?.[depGraphNode.name])
+                return {
+                  alias: rootAlias,
+                  name: depGraphNode.name,
+                  version: depGraphNode.version,
+                  dir: depGraphNode.dir,
+                  id: depGraphNode.id,
+                  dependencyType: (isDev && 'dev' || isOptional && 'optional' || 'prod') as 'dev' | 'optional' | 'prod',
+                  latest: opts.outdatedDependencies[depGraphNode.id],
+                  isExternalLink: false,
+                }
+              }),
+            ...opts.linkedDependenciesByProjectId[id].map(async (linkedDependency) => {
+              const dir = resolvePath(rootDir, linkedDependency.resolution.directory)
+              return {
+                alias: linkedDependency.alias,
+                name: linkedDependency.name,
+                version: linkedDependency.version,
+                dir,
+                id: linkedDependency.resolution.directory,
+                dependencyType: (linkedDependency.dev && 'dev' || linkedDependency.optional && 'optional' || 'prod') as 'dev' | 'optional' | 'prod',
+                isExternalLink: true,
+              }
+            }),
+          ]),
+        }]
+      }))
+    )
+    await linkDirectDeps(projectsToLink, { dedupe: opts.dedupeDirectDeps })
+  }
+
   return {
     currentLockfile,
     newDepPaths,

pkg-manager/core/test/install/dedupeDirectDeps.ts

@@ -2,6 +2,7 @@ import fs from 'fs'
 import path from 'path'
 import { preparePackages } from '@pnpm/prepare'
 import { mutateModules, type MutatedProject } from '@pnpm/core'
+import rimraf from '@zkochan/rimraf'
 import { testDefaults } from '../utils'
 
 test('dedupe direct dependencies', async () => {
@@ -102,3 +103,77 @@ test('dedupe direct dependencies', async () => {
   await projects['project-3'].hasNot('is-negative')
   expect(fs.existsSync('project-3/node_modules')).toBeFalsy()
 })
+
+test('dedupe direct dependencies after public hoisting', async () => {
+  const projects = preparePackages([
+    {
+      location: '',
+      package: { name: 'project-1' },
+    },
+    {
+      location: 'project-2',
+      package: { name: 'project-2' },
+    },
+  ])
+
+  const importers: MutatedProject[] = [
+    {
+      mutation: 'install',
+      rootDir: process.cwd(),
+    },
+    {
+      mutation: 'install',
+      rootDir: path.resolve('project-2'),
+    },
+  ]
+  const allProjects = [
+    {
+      buildIndex: 0,
+      manifest: {
+        name: 'project-1',
+        version: '1.0.0',
+
+        dependencies: {
+          '@pnpm.e2e/pkg-with-1-dep': '100.0.0',
+        },
+      },
+      rootDir: process.cwd(),
+    },
+    {
+      buildIndex: 0,
+      manifest: {
+        name: 'project-2',
+        version: '1.0.0',
+
+        dependencies: {
+          '@pnpm.e2e/dep-of-pkg-with-1-dep': '100.0.0',
+        },
+      },
+      rootDir: path.resolve('project-2'),
+    },
+  ]
+  const opts = await testDefaults({
+    allProjects,
+    dedupeDirectDeps: true,
+    publicHoistPattern: ['@pnpm.e2e/dep-of-pkg-with-1-dep'],
+  })
+  await mutateModules(importers, opts)
+  await projects['project-1'].has('@pnpm.e2e/dep-of-pkg-with-1-dep')
+  await projects['project-2'].hasNot('@pnpm.e2e/dep-of-pkg-with-1-dep')
+  expect(Array.from(fs.readdirSync('node_modules/@pnpm.e2e').sort())).toEqual([
+    'dep-of-pkg-with-1-dep',
+    'pkg-with-1-dep',
+  ])
+  expect(fs.existsSync('project-2/node_modules')).toBeFalsy()
+
+  // Test the same with headless install
+  await rimraf('node_modules')
+  await mutateModules(importers, { ...opts, frozenLockfile: true })
+  await projects['project-1'].has('@pnpm.e2e/dep-of-pkg-with-1-dep')
+  await projects['project-2'].hasNot('@pnpm.e2e/dep-of-pkg-with-1-dep')
+  expect(Array.from(fs.readdirSync('node_modules/@pnpm.e2e').sort())).toEqual([
+    'dep-of-pkg-with-1-dep',
+    'pkg-with-1-dep',
+  ])
+  expect(fs.existsSync('project-2/node_modules')).toBeFalsy()
+})

pnpm/test/install/hoist.ts

@@ -58,12 +58,13 @@ test('shamefully-hoist: applied to all the workspace projects when set to true i
   ])
 
   await writeYamlFile('pnpm-workspace.yaml', { packages: ['**', '!store/**'] })
-  await fs.writeFile('.npmrc', 'shamefully-hoist', 'utf8')
+  await fs.writeFile('.npmrc', 'shamefully-hoist=true', 'utf8')
 
-  await execPnpm(['recursive', 'install'])
+  await execPnpm(['install'])
 
   await projects.root.has('@pnpm.e2e/dep-of-pkg-with-1-dep')
   await projects.root.has('@pnpm.e2e/foo')
+  await projects.root.has('@pnpm.e2e/foobar')
   await projects.project.hasNot('@pnpm.e2e/foo')
-  await projects.project.has('@pnpm.e2e/foobar')
+  await projects.project.hasNot('@pnpm.e2e/foobar')
 })