diff --git a/.changeset/fluffy-readers-fry.md b/.changeset/fluffy-readers-fry.md new file mode 100644 index 0000000000..a8b96bc177 --- /dev/null +++ b/.changeset/fluffy-readers-fry.md @@ -0,0 +1,5 @@ +--- +"@pnpm/filter-lockfile": major +--- + +`filterLockfileByImportersAndEngine()` does not remove the skipped packages from the filtered lockfile. diff --git a/.changeset/twelve-camels-agree.md b/.changeset/twelve-camels-agree.md new file mode 100644 index 0000000000..71690cf7f8 --- /dev/null +++ b/.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. diff --git a/packages/filter-lockfile/src/filterLockfileByImportersAndEngine.ts b/packages/filter-lockfile/src/filterLockfileByImportersAndEngine.ts index 377110fe69..7f5f48857f 100644 --- a/packages/filter-lockfile/src/filterLockfileByImportersAndEngine.ts +++ b/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) diff --git a/packages/filter-lockfile/test/filterByImportersAndEngine.ts b/packages/filter-lockfile/test/filterByImportersAndEngine.ts index 236fe2f82f..4f6fc9dda7 100644 --- a/packages/filter-lockfile/test/filterByImportersAndEngine.ts +++ b/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: '' }, }, diff --git a/packages/headless/src/index.ts b/packages/headless/src/index.ts index cf9c7244ff..ab3717c8f9 100644 --- a/packages/headless/src/index.ts +++ b/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 diff --git a/packages/modules-cleaner/src/prune.ts b/packages/modules-cleaner/src/prune.ts index 52817694ab..50f9e24638 100644 --- a/packages/modules-cleaner/src/prune.ts +++ b/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 ): {[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) } diff --git a/packages/plugin-commands-rebuild/test/index.ts b/packages/plugin-commands-rebuild/test/index.ts index aae18687c8..006f4290a4 100644 --- a/packages/plugin-commands-rebuild/test/index.ts +++ b/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() }) diff --git a/packages/supi/src/install/link.ts b/packages/supi/src/install/link.ts index fbbdc6fa3a..3dd559145e 100644 --- a/packages/supi/src/install/link.ts +++ b/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, storeController: StoreController, virtualStoreDir: string, } ): Promise { - const wantedRelDepPaths = R.keys(wantedLockfile.packages) + const wantedRelDepPaths = R.difference(R.keys(wantedLockfile.packages), Array.from(opts.skipped)) let newDepPathsSet: Set if (opts.force) { diff --git a/packages/supi/test/install/optionalDependencies.ts b/packages/supi/test/install/optionalDependencies.ts index a917de90f4..866bb60c6c 100644 --- a/packages/supi/test/install/optionalDependencies.ts +++ b/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'))