fix: installing a new optional dependency

...that has an optional dependency

close #2663
PR #2667

.changeset/honest-steaks-cheat.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/resolve-dependencies": patch
+---
+
+Only add packages to the skipped set, when they are seen the first time.

.changeset/serious-plants-fix.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/prune-lockfile": patch
+---
+
+Don't remove optional dependencies of optional dependencies.

packages/prune-lockfile/src/index.ts

@@ -18,9 +18,9 @@ export function pruneSharedLockfile (
   }
 ) {
   const copiedPackages = !lockfile.packages ? {} : copyPackageSnapshots(lockfile.packages, {
-    devRelPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToRelDepPaths(deps.devDependencies || {}))),
-    optionalRelPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToRelDepPaths(deps.optionalDependencies || {}))),
-    prodRelPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToRelDepPaths(deps.dependencies || {}))),
+    devDepPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToDepPaths(deps.devDependencies ?? {}))),
+    optionalDepPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToDepPaths(deps.optionalDependencies ?? {}))),
+    prodDepPaths: R.unnest(R.values(lockfile.importers).map((deps) => resolvedDepsToDepPaths(deps.dependencies ?? {}))),
     warn: opts && opts.warn || ((msg: string) => undefined),
   })
 
@@ -44,7 +44,7 @@ export function pruneLockfile (
 ): Lockfile {
   const packages: PackageSnapshots = {}
   const importer = lockfile.importers[importerId]
-  const lockfileSpecs: ResolvedDependencies = importer.specifiers || {}
+  const lockfileSpecs: ResolvedDependencies = importer.specifiers ?? {}
   const optionalDependencies = R.keys(pkg.optionalDependencies)
   const dependencies = R.difference(R.keys(pkg.dependencies), optionalDependencies)
   const devDependencies = R.difference(R.difference(R.keys(pkg.devDependencies), optionalDependencies), dependencies)
@@ -111,106 +111,90 @@ export function pruneLockfile (
 function copyPackageSnapshots (
   originalPackages: PackageSnapshots,
   opts: {
-    devRelPaths: string[],
-    optionalRelPaths: string[],
-    prodRelPaths: string[],
+    devDepPaths: string[],
+    optionalDepPaths: string[],
+    prodDepPaths: string[],
     warn: (msg: string) => void,
   }
 ): PackageSnapshots {
-  const copiedPackages: PackageSnapshots = {}
-  const nonOptional = new Set<string>()
-  const notProdOnly = new Set<string>()
+  const copiedSnapshots: PackageSnapshots = {}
+  const ctx = {
+    copiedSnapshots,
+    nonOptional: new Set<string>(),
+    notProdOnly: new Set<string>(),
+    originalPackages,
+    walked: new Set<string>(),
+    warn: opts.warn,
+  }
 
-  copyDependencySubGraph(copiedPackages, opts.devRelPaths, originalPackages, new Set(), opts.warn, {
+  copyDependencySubGraph(ctx, opts.devDepPaths, {
     dev: true,
-    nonOptional,
-    notProdOnly,
+    optional: false,
   })
-
-  copyDependencySubGraph(copiedPackages, opts.prodRelPaths, originalPackages, new Set(), opts.warn, {
-    nonOptional,
-    notProdOnly,
-  })
-
-  copyDependencySubGraph(copiedPackages, opts.optionalRelPaths, originalPackages, new Set(), opts.warn, {
-    nonOptional,
-    notProdOnly,
+  copyDependencySubGraph(ctx, opts.optionalDepPaths, {
+    dev: false,
     optional: true,
   })
-
-  copyDependencySubGraph(copiedPackages, opts.devRelPaths, originalPackages, new Set(), opts.warn, {
-    dev: true,
-    nonOptional,
-    notProdOnly,
-    walkOptionals: true,
+  copyDependencySubGraph(ctx, opts.prodDepPaths, {
+    dev: false,
+    optional: false,
   })
 
-  copyDependencySubGraph(copiedPackages, opts.prodRelPaths, originalPackages, new Set(), opts.warn, {
-    nonOptional,
-    notProdOnly,
-    walkOptionals: true,
-  })
-
-  return copiedPackages
+  return copiedSnapshots
 }
 
-function resolvedDepsToRelDepPaths (deps: ResolvedDependencies) {
-  return Object.keys(deps)
-    .map((pkgName) => refToRelative(deps[pkgName], pkgName))
-    .filter((relPath) => relPath !== null) as string[]
+function resolvedDepsToDepPaths (deps: ResolvedDependencies) {
+  return Object.entries(deps)
+    .map(([alias, ref]) => refToRelative(ref, alias))
+    .filter((depPath) => depPath !== null) as string[]
 }
 
 function copyDependencySubGraph (
-  copiedSnapshots: PackageSnapshots,
-  depRelativePaths: string[],
-  originalPackages: PackageSnapshots,
-  walked: Set<string>,
-  warn: (msg: string) => void,
-  opts: {
-    dev?: boolean,
-    optional?: boolean,
+  ctx: {
+    copiedSnapshots: PackageSnapshots,
     nonOptional: Set<string>,
     notProdOnly: Set<string>,
-    walkOptionals?: boolean,
+    originalPackages: PackageSnapshots,
+    walked: Set<string>,
+    warn: (msg: string) => void,
+  },
+  depPaths: string[],
+  opts: {
+    dev: boolean,
+    optional: boolean,
   }
 ) {
-  for (const depRalativePath of depRelativePaths) {
-    if (walked.has(depRalativePath)) continue
-    walked.add(depRalativePath)
-    if (!originalPackages[depRalativePath]) {
+  for (const depPath of depPaths) {
+    const key = `${depPath}:${opts.optional}:${opts.dev}`
+    if (ctx.walked.has(key)) continue
+    ctx.walked.add(key)
+    if (!ctx.originalPackages[depPath]) {
       // local dependencies don't need to be resolved in pnpm-lock.yaml
       // except local tarball dependencies
-      if (depRalativePath.startsWith('link:') || depRalativePath.startsWith('file:') && !depRalativePath.endsWith('.tar.gz')) continue
+      if (depPath.startsWith('link:') || depPath.startsWith('file:') && !depPath.endsWith('.tar.gz')) continue
 
-      // NOTE: Warnings should not be printed for the current lockfile (node_modules/.lockfile.yaml).
-      // The current lockfile does not contain the skipped packages, so it may have missing resolutions
-      warn(`Cannot find resolution of ${depRalativePath} in lockfile`)
+      ctx.warn(`Cannot find resolution of ${depPath} in lockfile`)
       continue
     }
-    const depLockfile = originalPackages[depRalativePath]
-    copiedSnapshots[depRalativePath] = depLockfile
-    if (opts.optional && !opts.nonOptional.has(depRalativePath)) {
+    const depLockfile = ctx.originalPackages[depPath]
+    ctx.copiedSnapshots[depPath] = depLockfile
+    if (opts.optional && !ctx.nonOptional.has(depPath)) {
       depLockfile.optional = true
     } else {
-      opts.nonOptional.add(depRalativePath)
+      ctx.nonOptional.add(depPath)
       delete depLockfile.optional
     }
     if (opts.dev) {
-      opts.notProdOnly.add(depRalativePath)
+      ctx.notProdOnly.add(depPath)
       depLockfile.dev = true
     } else if (depLockfile.dev === true) { // keeping if dev is explicitly false
       delete depLockfile.dev
-    } else if (depLockfile.dev === undefined && !opts.notProdOnly.has(depRalativePath)) {
+    } else if (depLockfile.dev === undefined && !ctx.notProdOnly.has(depPath)) {
       depLockfile.dev = false
     }
-    const newDependencies = R.keys(depLockfile.dependencies)
-      .map((pkgName: string) => refToRelative((depLockfile.dependencies && depLockfile.dependencies[pkgName]) as string, pkgName))
-      .filter((relPath) => relPath !== null) as string[]
-    copyDependencySubGraph(copiedSnapshots, newDependencies, originalPackages, walked, warn, opts)
-    if (!opts.walkOptionals) continue
-    const newOptionalDependencies = R.keys(depLockfile.optionalDependencies)
-      .map((pkgName: string) => refToRelative((depLockfile.optionalDependencies && depLockfile.optionalDependencies[pkgName]) as string, pkgName))
-      .filter((relPath) => relPath !== null) as string[]
-    copyDependencySubGraph(copiedSnapshots, newOptionalDependencies, originalPackages, walked, warn, { ...opts, optional: true })
+    const newDependencies = resolvedDepsToDepPaths(depLockfile.dependencies ?? {})
+    copyDependencySubGraph(ctx, newDependencies, opts)
+    const newOptionalDependencies = resolvedDepsToDepPaths(depLockfile.optionalDependencies ?? {})
+    copyDependencySubGraph(ctx, newOptionalDependencies, { dev: opts.dev, optional: true })
   }
 }

packages/resolve-dependencies/src/resolveDependencies.ts

@@ -679,13 +679,13 @@ async function resolveDependency (
         pnpmVersion: ctx.pnpmVersion,
       })
     )
-  if (currentIsInstallable !== true || !parentIsInstallable) {
-    ctx.skipped.add(pkgResponse.body.id)
-  }
   const installable = parentIsInstallable && currentIsInstallable !== false
   const isNew = !ctx.resolvedPackagesByPackageId[pkgResponse.body.id]
 
   if (isNew) {
+    if (currentIsInstallable !== true || !parentIsInstallable) {
+      ctx.skipped.add(pkgResponse.body.id)
+    }
     progressLogger.debug({
       packageId: pkgResponse.body.id,
       requester: ctx.lockfileDir,

packages/supi/test/install/optionalDependencies.ts

@@ -341,8 +341,24 @@ test('optional subdependency is skipped', async (t: tape.Test) => {
   }
 })
 
+// Covers https://github.com/pnpm/pnpm/issues/2663
+test('optional subdependency of newly added optional dependency is skipped', async (t: tape.Test) => {
+  const project = prepareEmpty(t)
+  const reporter = sinon.spy()
+
+  const manifest = await addDependenciesToPackage({}, ['pkg-with-optional'], await testDefaults({ reporter, targetDependenciesField: 'optionalDependencies' }))
+
+  const modulesInfo = await readYamlFile<{ skipped: string[] }>(path.join('node_modules', '.modules.yaml'))
+  t.deepEqual(modulesInfo.skipped, ['/dep-of-optional-pkg/1.0.0', '/not-compatible-with-any-os/1.0.0'], 'optional subdep skipped')
+
+  const lockfile = await project.readLockfile()
+
+  t.equal(Object.keys(lockfile.packages).length, 3)
+  t.ok(lockfile.packages['/not-compatible-with-any-os/1.0.0'])
+})
+
 test('only that package is skipped which is an optional dependency only and not installable', async (t) => {
-  prepareEmpty(t)
+  const project = prepareEmpty(t)
   const reporter = sinon.spy()
 
   const manifest = await addDependenciesToPackage({}, [
@@ -356,6 +372,9 @@ test('only that package is skipped which is an optional dependency only and not
     t.deepEqual(modulesInfo.skipped, ['/not-compatible-with-any-os-and-has-peer/1.0.0_peer-c@1.0.0'])
   }
 
+  const lockfile = await project.readLockfile()
+  t.equal(typeof lockfile.packages['/dep-of-optional-pkg/1.0.0'].optional, 'undefined')
+
   await rimraf('node_modules')
 
   await mutateModules(