fix: in a workspace, also include missing deeply linked workspace pac…

…kages at headless installation (#5220)

Co-authored-by: Zoltan Kochan <z@kochan.io>

close #5034

.changeset/new-vans-guess.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/headless": patch
+"pnpm": patch
+---
+
+Also include missing deeply linked workspace packages at headless installation [#5034](https://github.com/pnpm/pnpm/issues/5034).

.changeset/silly-walls-shave.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/filter-lockfile": major
+---
+
+Breaking change to the API. Also include missing deeply linked workspace packages at headless installation.

fixtures/workspace-external-depends-deep/.npmrc

@@ -0,0 +1,5 @@
+link-workspace-packages = deep
+prefer-workspace-packages = true
+shared-workspace-lockfile = true
+save-workspace-protocol = rolling
+registry=http://localhost:4873

fixtures/workspace-external-depends-deep/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "root",
+  "version": "1.0.0",
+  "dependencies": {
+    "is-positive": "1.0.0"
+  }
+}

fixtures/workspace-external-depends-deep/packages/f/package.json

@@ -0,0 +1,9 @@
+{
+  "name": "@kenrick95/internal-f",
+  "version": "1.0.0",
+  "dependencies": {
+    "is-positive": "1.0.0",
+    "is-negative": "1.0.0"
+  }
+}
+

fixtures/workspace-external-depends-deep/packages/g/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@kenrick95/internal-g",
+  "version": "1.0.0",
+  "dependencies": {
+    "@pnpm.e2e/external-depend-on-internal-dep": "1.0.0",
+    "is-positive": "1.0.0"
+  }
+}

fixtures/workspace-external-depends-deep/pnpm-workspace.yaml

@@ -0,0 +1,2 @@
+packages:
+  - 'packages/**'

package.json

@@ -39,7 +39,7 @@
     "@commitlint/prompt-cli": "^17.1.2",
     "@pnpm/eslint-config": "workspace:*",
     "@pnpm/meta-updater": "0.2.0",
-    "@pnpm/registry-mock": "3.0.0",
+    "@pnpm/registry-mock": "3.1.0",
     "@pnpm/tsconfig": "workspace:*",
     "@types/jest": "^29.1.2",
     "@types/node": "^14.18.29",

packages/core/package.json

@@ -75,7 +75,7 @@
     "@pnpm/git-utils": "workspace:*",
     "@pnpm/package-store": "workspace:*",
     "@pnpm/prepare": "workspace:*",
-    "@pnpm/registry-mock": "3.0.0",
+    "@pnpm/registry-mock": "3.1.0",
     "@pnpm/store-path": "workspace:*",
     "@pnpm/test-fixtures": "workspace:*",
     "@types/fs-extra": "^9.0.13",

packages/filter-lockfile/src/filterLockfileByImportersAndEngine.ts

@@ -29,30 +29,27 @@ export default function filterByImportersAndEngine (
     lockfileDir: string
     skipped: Set<string>
   }
-): Lockfile {
-  const importerDeps = importerIds
-    .map((importerId) => lockfile.importers[importerId])
-    .map((importer) => ({
-      ...(opts.include.dependencies ? importer.dependencies : {}),
-      ...(opts.include.devDependencies ? importer.devDependencies : {}),
-      ...(opts.include.optionalDependencies ? importer.optionalDependencies : {}),
-    }))
-    .map(Object.entries)
-  const directDepPaths = unnest(importerDeps)
-    .map(([pkgName, ref]) => dp.refToRelative(ref, pkgName))
-    .filter((nodeId) => nodeId !== null) as string[]
+): { lockfile: Lockfile, selectedImporterIds: string[] } {
+  const importerIdSet = new Set(importerIds) as Set<string>
+
+  const directDepPaths = toImporterDepPaths(lockfile, importerIds, {
+    include: opts.include,
+    importerIdSet,
+  })
 
-  const packages = (lockfile.packages != null)
-    ? pickPkgsWithAllDeps(lockfile.packages, directDepPaths, {
-      currentEngine: opts.currentEngine,
-      engineStrict: opts.engineStrict,
-      failOnMissingDependencies: opts.failOnMissingDependencies,
-      include: opts.include,
-      includeIncompatiblePackages: opts.includeIncompatiblePackages === true,
-      lockfileDir: opts.lockfileDir,
-      skipped: opts.skipped,
-    })
-    : {}
+  const packages =
+    lockfile.packages != null
+      ? pickPkgsWithAllDeps(lockfile, directDepPaths, importerIdSet, {
+        currentEngine: opts.currentEngine,
+        engineStrict: opts.engineStrict,
+        failOnMissingDependencies: opts.failOnMissingDependencies,
+        include: opts.include,
+        includeIncompatiblePackages:
+            opts.includeIncompatiblePackages === true,
+        lockfileDir: opts.lockfileDir,
+        skipped: opts.skipped,
+      })
+      : {}
 
   const importers = importerIds.reduce((acc, importerId) => {
     acc[importerId] = filterImporter(lockfile.importers[importerId], opts.include)
@@ -68,15 +65,19 @@ export default function filterByImportersAndEngine (
   }, { ...lockfile.importers })
 
   return {
-    ...lockfile,
-    importers,
-    packages,
+    lockfile: {
+      ...lockfile,
+      importers,
+      packages,
+    },
+    selectedImporterIds: Array.from(importerIdSet),
   }
 }
 
 function pickPkgsWithAllDeps (
-  pkgSnapshots: PackageSnapshots,
+  lockfile: Lockfile,
   depPaths: string[],
+  importerIdSet: Set<string>,
   opts: {
     currentEngine: {
       nodeVersion: string
@@ -91,14 +92,15 @@ function pickPkgsWithAllDeps (
   }
 ) {
   const pickedPackages = {} as PackageSnapshots
-  pkgAllDeps({ pkgSnapshots, pickedPackages }, depPaths, true, opts)
+  pkgAllDeps({ lockfile, pickedPackages, importerIdSet }, depPaths, true, opts)
   return pickedPackages
 }
 
 function pkgAllDeps (
   ctx: {
-    pkgSnapshots: PackageSnapshots
+    lockfile: Lockfile
     pickedPackages: PackageSnapshots
+    importerIdSet: Set<string>
   },
   depPaths: string[],
   parentIsInstallable: boolean,
@@ -117,7 +119,7 @@ function pkgAllDeps (
 ) {
   for (const depPath of depPaths) {
     if (ctx.pickedPackages[depPath]) continue
-    const pkgSnapshot = ctx.pkgSnapshots[depPath]
+    const pkgSnapshot = ctx.lockfile.packages![depPath]
     if (!pkgSnapshot && !depPath.startsWith('link:')) {
       if (opts.failOnMissingDependencies) {
         throw new LockfileMissingDependencyError(depPath)
@@ -140,13 +142,15 @@ function pkgAllDeps (
         libc: pkgSnapshot.libc,
       }
       // TODO: depPath is not the package ID. Should be fixed
-      installable = opts.includeIncompatiblePackages || packageIsInstallable(pkgSnapshot.id ?? depPath, pkg, {
-        engineStrict: opts.engineStrict,
-        lockfileDir: opts.lockfileDir,
-        nodeVersion: opts.currentEngine.nodeVersion,
-        optional: pkgSnapshot.optional === true,
-        pnpmVersion: opts.currentEngine.pnpmVersion,
-      }) !== false
+      installable =
+        opts.includeIncompatiblePackages ||
+        packageIsInstallable(pkgSnapshot.id ?? depPath, pkg, {
+          engineStrict: opts.engineStrict,
+          lockfileDir: opts.lockfileDir,
+          nodeVersion: opts.currentEngine.nodeVersion,
+          optional: pkgSnapshot.optional === true,
+          pnpmVersion: opts.currentEngine.pnpmVersion,
+        }) !== false
       if (!installable) {
         if (!ctx.pickedPackages[depPath] && pkgSnapshot.optional === true) {
           opts.skipped.add(depPath)
@@ -156,14 +160,69 @@ function pkgAllDeps (
       }
     }
     ctx.pickedPackages[depPath] = pkgSnapshot
-    const nextRelDepPaths = Object.entries(
-      {
-        ...pkgSnapshot.dependencies,
-        ...(opts.include.optionalDependencies ? pkgSnapshot.optionalDependencies : {}),
+    const { depPaths: nextRelDepPaths, importerIds: additionalImporterIds } = parseDepRefs(Object.entries({
+      ...pkgSnapshot.dependencies,
+      ...(opts.include.optionalDependencies
+        ? pkgSnapshot.optionalDependencies
+        : {}),
+    }), ctx.lockfile)
+    additionalImporterIds.forEach((importerId) => ctx.importerIdSet.add(importerId))
+    nextRelDepPaths.push(
+      ...toImporterDepPaths(ctx.lockfile, additionalImporterIds, {
+        include: opts.include,
+        importerIdSet: ctx.importerIdSet,
       })
-      .map(([pkgName, ref]) => dp.refToRelative(ref, pkgName))
-      .filter((nodeId) => nodeId !== null) as string[]
-
+    )
     pkgAllDeps(ctx, nextRelDepPaths, installable, opts)
   }
 }
+
+function toImporterDepPaths (
+  lockfile: Lockfile,
+  importerIds: string[],
+  opts: {
+    include: { [dependenciesField in DependenciesField]: boolean }
+    importerIdSet: Set<string>
+  }
+): string[] {
+  const importerDeps = importerIds
+    .map(importerId => lockfile.importers[importerId])
+    .map(importer => ({
+      ...(opts.include.dependencies ? importer.dependencies : {}),
+      ...(opts.include.devDependencies ? importer.devDependencies : {}),
+      ...(opts.include.optionalDependencies
+        ? importer.optionalDependencies
+        : {}),
+    }))
+    .map(Object.entries)
+
+  const { depPaths, importerIds: nextImporterIds } = parseDepRefs(unnest(importerDeps), lockfile)
+
+  if (!nextImporterIds.length) {
+    return depPaths
+  }
+  nextImporterIds.forEach((importerId) => {
+    opts.importerIdSet.add(importerId)
+  })
+  return [
+    ...depPaths,
+    ...toImporterDepPaths(lockfile, nextImporterIds, opts),
+  ]
+}
+
+function parseDepRefs (refsByPkgNames: Array<[string, string]>, lockfile: Lockfile) {
+  return refsByPkgNames
+    .reduce((acc, [pkgName, ref]) => {
+      if (ref.startsWith('link:')) {
+        const importerId = ref.substring(5)
+        if (lockfile.importers[importerId]) {
+          acc.importerIds.push(importerId)
+        }
+        return acc
+      }
+      const depPath = dp.refToRelative(ref, pkgName)
+      if (depPath == null) return acc
+      acc.depPaths.push(depPath)
+      return acc
+    }, { depPaths: [] as string[], importerIds: [] as string[] })
+}