From a6c2ce19a42456cfdf0b08e4802aee879d46fd1f Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 7 Mar 2020 11:59:00 +0200 Subject: [PATCH] fix: proper hoisting when doing some actions on a subset of workspace projects PR #2403 close #2340 --- .../src/filterLockfileByImporters.ts | 4 - .../filter-lockfile/test/filterByImporters.ts | 97 +++++++++++++++++++ packages/hoist/src/index.ts | 2 +- packages/supi/src/install/link.ts | 64 ++++++------ packages/supi/test/install/hoist.ts | 82 ++++++++++++++++ .../supi/test/install/multipleImporters.ts | 74 ++++++++++++++ 6 files changed, 284 insertions(+), 39 deletions(-) diff --git a/packages/filter-lockfile/src/filterLockfileByImporters.ts b/packages/filter-lockfile/src/filterLockfileByImporters.ts index 29bf231e6c..1d732a1800 100644 --- a/packages/filter-lockfile/src/filterLockfileByImporters.ts +++ b/packages/filter-lockfile/src/filterLockfileByImporters.ts @@ -9,7 +9,6 @@ import pnpmLogger from '@pnpm/logger' import { DependenciesField, Registries } from '@pnpm/types' import R = require('ramda') import filterImporter from './filterImporter' -import filterLockfile from './filterLockfile' const logger = pnpmLogger('lockfile') @@ -23,9 +22,6 @@ export default function filterByImporters ( failOnMissingDependencies: boolean, }, ): Lockfile { - if (R.equals(importerIds.sort(), R.keys(lockfile.importers).sort())) { - return filterLockfile(lockfile, opts) - } const packages = {} as PackageSnapshots if (lockfile.packages) { pkgAllDeps( diff --git a/packages/filter-lockfile/test/filterByImporters.ts b/packages/filter-lockfile/test/filterByImporters.ts index c04fbfaece..4090a882bc 100644 --- a/packages/filter-lockfile/test/filterByImporters.ts +++ b/packages/filter-lockfile/test/filterByImporters.ts @@ -361,3 +361,100 @@ test('filterByImporters(): do not include skipped packages', (t) => { }) t.end() }) + +test('filterByImporters(): exclude orphan packages', (t) => { + const filteredLockfile = filterLockfileByImporters( + { + importers: { + 'project-1': { + dependencies: { + 'prod-dep': '1.0.0', + }, + specifiers: { + 'prod-dep': '^1.0.0', + }, + }, + 'project-2': { + dependencies: { + 'project-2-prod-dep': '1.0.0', + }, + specifiers: { + 'project-2-prod-dep': '^1.0.0', + }, + }, + }, + lockfileVersion: 5.1, + packages: { + '/orphan/1.0.0': { + resolution: { integrity: '' }, + }, + '/prod-dep-dep/1.0.0': { + resolution: { integrity: '' }, + }, + '/prod-dep/1.0.0': { + dependencies: { + 'prod-dep-dep': '1.0.0', + }, + resolution: { integrity: '' }, + }, + '/project-2-prod-dep/1.0.0': { + resolution: { integrity: '' }, + }, + }, + }, + ['project-1', 'project-2'], + { + failOnMissingDependencies: true, + include: { + dependencies: true, + devDependencies: true, + optionalDependencies: true, + }, + registries: { + default: 'https://registry.npmjs.org/', + }, + skipped: new Set(), + }, + ) + + t.deepEqual(filteredLockfile, { + importers: { + 'project-1': { + dependencies: { + 'prod-dep': '1.0.0', + }, + devDependencies: {}, + optionalDependencies: {}, + specifiers: { + 'prod-dep': '^1.0.0', + }, + }, + 'project-2': { + dependencies: { + 'project-2-prod-dep': '1.0.0', + }, + devDependencies: {}, + optionalDependencies: {}, + specifiers: { + 'project-2-prod-dep': '^1.0.0', + }, + }, + }, + lockfileVersion: 5.1, + packages: { + '/prod-dep-dep/1.0.0': { + resolution: { integrity: '' }, + }, + '/prod-dep/1.0.0': { + dependencies: { + 'prod-dep-dep': '1.0.0', + }, + resolution: { integrity: '' }, + }, + '/project-2-prod-dep/1.0.0': { + resolution: { integrity: '' }, + }, + }, + }) + t.end() +}) diff --git a/packages/hoist/src/index.ts b/packages/hoist/src/index.ts index 2622856a24..85c5651839 100644 --- a/packages/hoist/src/index.ts +++ b/packages/hoist/src/index.ts @@ -55,7 +55,7 @@ export default async function hoistByLockfile ( ), ] - const aliasesByDependencyPath = await hoistGraph(deps, opts.lockfile.importers['.'].specifiers, { + const aliasesByDependencyPath = await hoistGraph(deps, opts.lockfile.importers['.']?.specifiers ?? {}, { dryRun: false, match, modulesDir: opts.modulesDir, diff --git a/packages/supi/src/install/link.ts b/packages/supi/src/install/link.ts index c24411c43e..219e9b2999 100644 --- a/packages/supi/src/install/link.ts +++ b/packages/supi/src/install/link.ts @@ -270,18 +270,7 @@ export default async function linkPackages ( opts.makePartialCurrentLockfile || !allImportersIncluded ) { - const filteredCurrentLockfile = allImportersIncluded - ? opts.currentLockfile - : filterLockfileByImporters( - opts.currentLockfile, - Object.keys(newWantedLockfile.importers) - .filter((projectId) => !projectIds.includes(projectId) && opts.currentLockfile.importers[projectId]), - { - ...filterOpts, - failOnMissingDependencies: false, - }, - ) - const packages = filteredCurrentLockfile.packages || {} + const packages = opts.currentLockfile.packages || {} if (newWantedLockfile.packages) { for (const relDepPath in newWantedLockfile.packages) { // tslint:disable-line:forin const depPath = dp.resolve(opts.registries, relDepPath) @@ -294,7 +283,17 @@ export default async function linkPackages ( acc[projectId] = newWantedLockfile.importers[projectId] return acc }, opts.currentLockfile.importers) - currentLockfile = { ...newWantedLockfile, packages, importers: projects } + currentLockfile = filterLockfileByImporters( + { + ...newWantedLockfile, + importers: projects, + packages, + }, + Object.keys(projects), { + ...filterOpts, + failOnMissingDependencies: false, + }, + ) } else if ( opts.include.dependencies && opts.include.devDependencies && @@ -306,27 +305,24 @@ export default async function linkPackages ( currentLockfile = newCurrentLockfile } - let newHoistedAliases: {[depPath: string]: string[]} = {} - if (newDepPaths.length > 0 || removedDepPaths.size > 0) { - const rootImporterWithFlatModules = opts.hoistPattern && projects.find(({ id }) => id === '.') - if (rootImporterWithFlatModules) { - newHoistedAliases = await hoist(matcher(opts.hoistPattern!), { - getIndependentPackageLocation: opts.independentLeaves - ? async (packageId: string, packageName: string) => { - const { dir } = await opts.storeController.getPackageLocation(packageId, packageName, { - lockfileDir: opts.lockfileDir, - targetEngine: opts.sideEffectsCacheRead && ENGINE_NAME || undefined, - }) - return dir - } - : undefined, - lockfile: currentLockfile, - lockfileDir: opts.lockfileDir, - modulesDir: opts.hoistedModulesDir, - registries: opts.registries, - virtualStoreDir: opts.virtualStoreDir, - }) - } + let newHoistedAliases: Record = {} + if (opts.hoistPattern && (newDepPaths.length > 0 || removedDepPaths.size > 0)) { + newHoistedAliases = await hoist(matcher(opts.hoistPattern!), { + getIndependentPackageLocation: opts.independentLeaves + ? async (packageId: string, packageName: string) => { + const { dir } = await opts.storeController.getPackageLocation(packageId, packageName, { + lockfileDir: opts.lockfileDir, + targetEngine: opts.sideEffectsCacheRead && ENGINE_NAME || undefined, + }) + return dir + } + : undefined, + lockfile: currentLockfile, + lockfileDir: opts.lockfileDir, + modulesDir: opts.hoistedModulesDir, + registries: opts.registries, + virtualStoreDir: opts.virtualStoreDir, + }) } if (!opts.dryRun) { diff --git a/packages/supi/test/install/hoist.ts b/packages/supi/test/install/hoist.ts index dbfe76c2ea..305fff612f 100644 --- a/packages/supi/test/install/hoist.ts +++ b/packages/supi/test/install/hoist.ts @@ -1,3 +1,4 @@ +import assertProject from '@pnpm/assert-project' import { prepareEmpty, preparePackages } from '@pnpm/prepare' import { REGISTRY_MOCK_PORT } from '@pnpm/registry-mock' import rimraf = require('@zkochan/rimraf') @@ -384,3 +385,84 @@ test('hoist-pattern: hoist all dependencies to the virtual store node_modules', await projects['package'].hasNot('foo') await projects['package'].hasNot('bar') }) + +test('hoist when updating in one of the workspace projects', async (t) => { + await addDistTag('dep-of-pkg-with-1-dep', '100.0.0', 'latest') + + const workspaceRootManifest = { + name: 'root', + + dependencies: { + 'pkg-with-1-dep': '100.0.0', + }, + } + const workspacePackageManifest = { + name: 'package', + + dependencies: { + 'foo': '100.0.0', + }, + } + const projects = preparePackages(t, [ + { + location: '.', + package: workspaceRootManifest, + }, + { + location: 'package', + package: workspacePackageManifest, + }, + ]) + + const mutatedProjects: MutatedProject[] = [ + { + buildIndex: 0, + manifest: workspaceRootManifest, + mutation: 'install', + rootDir: process.cwd(), + }, + { + buildIndex: 0, + manifest: workspacePackageManifest, + mutation: 'install', + rootDir: path.resolve('package'), + }, + ] + await mutateModules(mutatedProjects, await testDefaults({ hoistPattern: '*' })) + + const rootNodeModules = assertProject(t, process.cwd()) + { + const modulesManifest = await rootNodeModules.readModulesManifest() + t.deepEqual(modulesManifest?.hoistedAliases, { + 'localhost+4873/dep-of-pkg-with-1-dep/100.0.0': ['dep-of-pkg-with-1-dep'], + 'localhost+4873/foo/100.0.0': ['foo'], + }) + } + + await mutateModules([ + { + ...mutatedProjects[0], + dependencySelectors: ['foo@100.1.0'], + mutation: 'installSome', + }, + ], await testDefaults({ hoistPattern: '*', pruneLockfileImporters: false })) + + const lockfile = await rootNodeModules.readCurrentLockfile() + + t.deepEqual( + Object.keys(lockfile.packages), + [ + '/dep-of-pkg-with-1-dep/100.0.0', + '/foo/100.0.0', + '/foo/100.1.0', + '/pkg-with-1-dep/100.0.0', + ], + ) + + { + const modulesManifest = await rootNodeModules.readModulesManifest() + t.deepEqual(modulesManifest?.hoistedAliases, { + 'localhost+4873/dep-of-pkg-with-1-dep/100.0.0': ['dep-of-pkg-with-1-dep'], + }) + } +}) diff --git a/packages/supi/test/install/multipleImporters.ts b/packages/supi/test/install/multipleImporters.ts index 3ae157979c..df6cd1e172 100644 --- a/packages/supi/test/install/multipleImporters.ts +++ b/packages/supi/test/install/multipleImporters.ts @@ -70,6 +70,80 @@ test('install only the dependencies of the specified importer', async (t) => { await rootNodeModules.hasNot(`.pnpm/localhost+${REGISTRY_MOCK_PORT}/is-negative/1.0.0`) }) +test('install only the dependencies of the specified importer. The current lockfile has importers that do not exist anymore', async (t) => { + preparePackages(t, [ + { + location: 'project-1', + package: { name: 'project-1' }, + }, + { + location: 'project-2', + package: { name: 'project-2' }, + }, + { + location: 'project-3', + package: { name: 'project-3' }, + }, + ]) + + const importers: MutatedProject[] = [ + { + buildIndex: 0, + manifest: { + name: 'project-1', + version: '1.0.0', + + dependencies: { + 'is-positive': '1.0.0', + }, + }, + mutation: 'install', + rootDir: path.resolve('project-1'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-2', + version: '1.0.0', + + dependencies: { + 'is-negative': '1.0.0', + }, + }, + mutation: 'install', + rootDir: path.resolve('project-2'), + }, + { + buildIndex: 0, + manifest: { + name: 'project-3', + version: '1.0.0', + + dependencies: { + 'foobar': '100.0.0', + }, + }, + mutation: 'install', + rootDir: path.resolve('project-3'), + }, + ] + await mutateModules(importers, await testDefaults({ hoistPattern: '*' })) + await mutateModules(importers.slice(0, 2), await testDefaults({ lockfileOnly: true, pruneLockfileImporters: true })) + + await mutateModules([ + { + ...importers[0], + dependencySelectors: ['pkg-with-1-dep'], + mutation: 'installSome', + }, + ], await testDefaults({ hoistPattern: '*' })) + + const rootNodeModules = assertProject(t, process.cwd()) + const currentLockfile = await rootNodeModules.readCurrentLockfile() + t.ok(currentLockfile.importers['project-3']) + t.ok(currentLockfile.packages['/foobar/100.0.0']) +}) + test('dependencies of other importers are not pruned when installing for a subset of importers', async (t) => { const projects = preparePackages(t, [ {