Fix installaltion without lockfile when an up-to-date node_modules is present

.changeset/fluffy-readers-fry.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/filter-lockfile": major
+---
+
+`filterLockfileByImportersAndEngine()` does not remove the skipped packages from the filtered lockfile.

.changeset/twelve-camels-agree.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/resolve-dependencies": patch
+"supi": patch
+---
+
+The lockfile should be recreated correctly when an up-to-date `node_modules` is present.
+The recreated lockfile should contain all the skipped optional dependencies.

packages/filter-lockfile/src/filterLockfileByImportersAndEngine.ts

@@ -127,7 +127,7 @@ function pkgAllDeps (
     let installable!: boolean
     if (!parentIsInstallable) {
       installable = false
-      if (!ctx.pickedPackages[relDepPath]) {
+      if (!ctx.pickedPackages[relDepPath] && pkgSnapshot.optional === true) {
         opts.skipped.add(relDepPath)
       }
     } else {
@@ -146,20 +146,23 @@ function pkgAllDeps (
         pnpmVersion: opts.currentEngine.pnpmVersion,
       }) !== false
       if (!installable) {
-        if (!ctx.pickedPackages[relDepPath]) {
+        if (!ctx.pickedPackages[relDepPath] && pkgSnapshot.optional === true) {
           opts.skipped.add(relDepPath)
         }
       } else {
         opts.skipped.delete(relDepPath)
-        ctx.pickedPackages[relDepPath] = pkgSnapshot
       }
     }
+    ctx.pickedPackages[relDepPath] = pkgSnapshot
     const nextRelDepPaths = R.toPairs(
       {
         ...pkgSnapshot.dependencies,
         ...(opts.include.optionalDependencies && pkgSnapshot.optionalDependencies || {}),
       })
-      .map(([pkgName, ref]) => dp.refToRelative(ref, pkgName))
+      .map(([pkgName, ref]) => {
+        if (pkgSnapshot.peerDependencies?.[pkgName]) return null
+        return dp.refToRelative(ref, pkgName)
+      })
       .filter((nodeId) => nodeId !== null) as string[]
 
     pkgAllDeps(ctx, nextRelDepPaths, installable, opts)

packages/filter-lockfile/test/filterByImportersAndEngine.ts

@@ -109,6 +109,7 @@ test('filterByImportersAndEngine(): skip packages that are not installable', (t)
         },
         optionalDependencies: {
           'not-skipped-optional': '1.0.0',
+          'optional-dep': '1.0.0',
         },
         specifiers: {
           'dev-dep': '^1.0.0',
@@ -135,10 +136,25 @@ test('filterByImportersAndEngine(): skip packages that are not installable', (t)
         dev: true,
         resolution: { integrity: '' },
       },
+      '/foo/1.0.0': {
+        optional: true,
+        resolution: { integrity: '' },
+      },
       '/not-skipped-optional/1.0.0': {
         optional: true,
         resolution: { integrity: '' },
       },
+      '/optional-dep/1.0.0': {
+        dependencies: {
+          'bar': '1.0.0',
+          'foo': '1.0.0',
+        },
+        engines: {
+          node: '1000',
+        },
+        optional: true,
+        resolution: { integrity: '' },
+      },
       '/prod-dep-dep/1.0.0': {
         resolution: { integrity: '' },
       },

packages/headless/src/index.ts

@@ -491,6 +491,7 @@ async function lockfileToDepGraph (
     const pkgSnapshotByLocation = {}
     await Promise.all(
       Object.keys(lockfile.packages).map(async (depPath) => {
+        if (opts.skipped.has(depPath)) return
         const pkgSnapshot = lockfile.packages![depPath]
         // TODO: optimize. This info can be already returned by pkgSnapshotToResolution()
         const pkgName = nameVerFromPkgSnapshot(depPath, pkgSnapshot).name

packages/modules-cleaner/src/prune.ts

@@ -97,9 +97,9 @@ export default async function prune (
   // we may only prune dependencies that are used only by that subset of importers.
   // Otherwise, we would break the node_modules.
   const currentPkgIdsByDepPaths = R.equals(selectedImporterIds, Object.keys(opts.wantedLockfile.importers))
-    ? getPkgsDepPaths(opts.registries, opts.currentLockfile.packages ?? {})
+    ? getPkgsDepPaths(opts.registries, opts.currentLockfile.packages ?? {}, opts.skipped)
     : getPkgsDepPathsOwnedOnlyByImporters(selectedImporterIds, opts.registries, opts.currentLockfile, opts.include, opts.skipped)
-  const wantedPkgIdsByDepPaths = getPkgsDepPaths(opts.registries, wantedLockfile.packages || {})
+  const wantedPkgIdsByDepPaths = getPkgsDepPaths(opts.registries, wantedLockfile.packages || {}, opts.skipped)
 
   const oldDepPaths = Object.keys(currentPkgIdsByDepPaths)
   const newDepPaths = Object.keys(wantedPkgIdsByDepPaths)
@@ -162,10 +162,12 @@ function mergeDependencies (projectSnapshot: ProjectSnapshot): { [depName: strin
 
 function getPkgsDepPaths (
   registries: Registries,
-  packages: PackageSnapshots
+  packages: PackageSnapshots,
+  skipped: Set<string>
 ): {[relDepPath: string]: string} {
   const pkgIdsByDepPath = {}
   for (const relDepPath of Object.keys(packages)) {
+    if (skipped.has(relDepPath)) continue
     pkgIdsByDepPath[relDepPath] = packageIdFromSnapshot(relDepPath, packages[relDepPath], registries)
   }
   return pkgIdsByDepPath
@@ -193,5 +195,5 @@ function getPkgsDepPathsOwnedOnlyByImporters (
       skipped,
     })
   const packagesOfSelectedOnly = R.pickAll(R.difference(Object.keys(selected.packages!), Object.keys(other.packages!)), selected.packages!) as PackageSnapshots
-  return getPkgsDepPaths(registries, packagesOfSelectedOnly)
+  return getPkgsDepPaths(registries, packagesOfSelectedOnly, skipped)
 }

packages/plugin-commands-rebuild/test/index.ts

@@ -343,10 +343,5 @@ test(`rebuild should not fail on incomplete ${WANTED_LOCKFILE}`, async (t) => {
     storeDir,
   }, [])
 
-  t.ok(reporter.calledWithMatch({
-    level: 'debug',
-    message: `No entry for "/not-compatible-with-any-os/1.0.0" in ${WANTED_LOCKFILE}`,
-    name: 'pnpm',
-  }), 'missing package reported')
   t.end()
 })

packages/supi/src/install/link.ts

@@ -176,6 +176,7 @@ export default async function linkPackages (
   const newCurrentLockfile = filterLockfileByImporters(newWantedLockfile, projectIds, {
     ...filterOpts,
     failOnMissingDependencies: true,
+    skipped: new Set(),
   })
   const newDepPaths = await linkNewPackages(
     filterLockfileByImporters(opts.currentLockfile, projectIds, {
@@ -191,6 +192,7 @@ export default async function linkPackages (
       optional: opts.include.optionalDependencies,
       registries: opts.registries,
       sideEffectsCacheRead: opts.sideEffectsCacheRead,
+      skipped: opts.skipped,
       storeController: opts.storeController,
       virtualStoreDir: opts.virtualStoreDir,
     }
@@ -367,11 +369,12 @@ async function linkNewPackages (
     registries: Registries,
     lockfileDir: string,
     sideEffectsCacheRead: boolean,
+    skipped: Set<string>,
     storeController: StoreController,
     virtualStoreDir: string,
   }
 ): Promise<string[]> {
-  const wantedRelDepPaths = R.keys(wantedLockfile.packages)
+  const wantedRelDepPaths = R.difference(R.keys(wantedLockfile.packages), Array.from(opts.skipped))
 
   let newDepPathsSet: Set<string>
   if (opts.force) {

packages/supi/test/install/optionalDependencies.ts

@@ -78,7 +78,7 @@ test('skip optional dependency that does not support the current OS', async (t:
 
   const currentLockfile = await project.readCurrentLockfile()
 
-  t.ok(R.isEmpty(currentLockfile.packages || {}), 'current lockfile does not contain skipped packages')
+  t.deepEqual(currentLockfile.packages, lockfile.packages, 'current lockfile contains skipped packages')
 
   const modulesInfo = await readYamlFile<{skipped: string[]}>(path.join('node_modules', '.modules.yaml'))
   t.deepEquals(modulesInfo.skipped, [
@@ -203,7 +203,7 @@ test('don\'t skip optional dependency that does not support the current OS when
 })
 
 test('optional subdependency is skipped', async (t: tape.Test) => {
-  prepareEmpty(t)
+  const project = prepareEmpty(t)
   const reporter = sinon.spy()
 
   const manifest = await addDependenciesToPackage({}, ['pkg-with-optional', 'dep-of-optional-pkg'], await testDefaults({ reporter }))
@@ -227,6 +227,30 @@ test('optional subdependency is skipped', async (t: tape.Test) => {
   const reportedTimes = reporter.withArgs(logMatcher).callCount
   t.equal(reportedTimes, 1, 'skipping optional dependency is logged')
 
+  t.comment('recreate the lockfile with optional dependencies present')
+
+  t.ok(await exists('pnpm-lock.yaml'))
+  await rimraf('pnpm-lock.yaml')
+
+  await mutateModules(
+    [
+      {
+        buildIndex: 0,
+        manifest,
+        mutation: 'install',
+        rootDir: process.cwd(),
+      },
+    ],
+    await testDefaults()
+  )
+
+  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'])
+
+  t.comment('forced headless install should install non-compatible optional deps')
+
   // TODO: move next case to @pnpm/headless tests
   await mutateModules(
     [
@@ -252,7 +276,11 @@ test('only that package is skipped which is an optional dependency only and not
   prepareEmpty(t)
   const reporter = sinon.spy()
 
-  const manifest = await addDependenciesToPackage({}, ['peer-c@1.0.1', 'has-optional-dep-with-peer', 'not-compatible-with-any-os-and-has-peer'], await testDefaults({ reporter }))
+  const manifest = await addDependenciesToPackage({}, [
+    'peer-c@1.0.1',
+    'has-optional-dep-with-peer',
+    'not-compatible-with-any-os-and-has-peer',
+  ], await testDefaults({ reporter }))
 
   {
     const modulesInfo = await readYamlFile<{ skipped: string[] }>(path.join('node_modules', '.modules.yaml'))