From 6eabef10f41aa76642e7e4ac12fad281ea6df6e4 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Tue, 30 Jul 2019 00:36:18 +0300 Subject: [PATCH] fix: flat node_modules inside a workspace Flat node_modules is only allowed in the workspace root. shamefully-flatten should only hoists dependencies of the root workspace package. close #1928 PR #1931 BREAKING CHANGE: @pnpm/shamefully-flatten --- packages/default-reporter/src/reportError.ts | 26 +++++ .../default-reporter/test/reportingErrors.ts | 36 +++++++ packages/headless/src/index.ts | 42 ++++---- packages/pnpm/src/cmd/recursive/index.ts | 5 +- .../pnpm/test/install/shamefullyFlatten.ts | 38 ++++++- packages/pnpm/test/monorepo/index.ts | 3 +- packages/shamefully-flatten/src/index.ts | 13 ++- packages/supi/src/install/index.ts | 17 ++++ packages/supi/src/install/link.ts | 43 ++++---- .../supi/test/install/shamefullyFlatten.ts | 99 ++++++++++++++++++- 10 files changed, 266 insertions(+), 56 deletions(-) diff --git a/packages/default-reporter/src/reportError.ts b/packages/default-reporter/src/reportError.ts index 1c368b01d7..0689824fb1 100644 --- a/packages/default-reporter/src/reportError.ts +++ b/packages/default-reporter/src/reportError.ts @@ -43,6 +43,8 @@ export default function reportError (logObj: Log) { return reportLifecycleError(logObj['message']) case 'ERR_PNPM_UNSUPPORTED_ENGINE': return reportEngineError(err, logObj['message']) + case 'ERR_PNPM_SHAMEFULLY_FLATTEN_NOT_IN_LOCKFILE_DIR': + return reportShamefullyFlattenNotInLockfileDirError(logObj['message']) default: // Errors with known error codes are printed w/o stack trace if (err.code && err.code.startsWith && err.code.startsWith('ERR_PNPM_')) { @@ -270,3 +272,27 @@ function reportEngineError ( } return output || formatErrorSummary(err.message) } + +function reportShamefullyFlattenNotInLockfileDirError ( + msg: { + lockfileDirectory: string, + message: string, + shamefullyFlattenDirectory: string, + }, +) { + return stripIndent` + ${formatErrorSummary(`Shamefully flatten can be only used in the lockfile directory`)} + + You were trying to install a flat node_modules in ${msg.shamefullyFlattenDirectory} + Flat node_modules is only possible in the directory that contains the lockfile (pnpm-lock.yaml). + In your case: ${msg.lockfileDirectory} + + If you really need a flat node_modules in ${msg.shamefullyFlattenDirectory}, + then you should use a dedicated lockfile. + Create a .npmrc in the root of your workspace with the following content: + shared-workspace-lockfile=false + + If you don't need a flat node_modules, remove shamefully-flatten=true + from the .npmrc file in ${msg.shamefullyFlattenDirectory} + ` +} diff --git a/packages/default-reporter/test/reportingErrors.ts b/packages/default-reporter/test/reportingErrors.ts index 5ac1114cee..6c94a830bf 100644 --- a/packages/default-reporter/test/reportingErrors.ts +++ b/packages/default-reporter/test/reportingErrors.ts @@ -331,3 +331,39 @@ test('prints unsupported pnpm and Node versions error', async (t) => { err['current'] = { pnpm: '3.0.0', node: '10.0.0' } logger.error(err, err) }) + +test('prints shamefully-flatten is only allowed in the root workspace package error', async (t) => { + const output$ = toOutput$({ + context: { argv: ['install'] }, + streamParser: createStreamParser(), + }) + + t.plan(1) + + output$.take(1).map(normalizeNewline).subscribe({ + complete: () => t.end(), + error: t.end, + next: output => { + t.equal(output, stripIndent` + ${ERROR} ${chalk.red('Shamefully flatten can be only used in the lockfile directory')} + + You were trying to install a flat node_modules in /home/zoltan/repo/package + Flat node_modules is only possible in the directory that contains the lockfile (pnpm-lock.yaml). + In your case: /home/zoltan/repo + + If you really need a flat node_modules in /home/zoltan/repo/package, + then you should use a dedicated lockfile. + Create a .npmrc in the root of your workspace with the following content: + shared-workspace-lockfile=false + + If you don't need a flat node_modules, remove shamefully-flatten=true + from the .npmrc file in /home/zoltan/repo/package + `) + }, + }) + + const err = new PnpmError('SHAMEFULLY_FLATTEN_NOT_IN_LOCKFILE_DIR', 'Shamefully flatten can be only used in the lockfile directory') + err['shamefullyFlattenDirectory'] = '/home/zoltan/repo/package' + err['lockfileDirectory'] = '/home/zoltan/repo' + logger.error(err, err) +}) diff --git a/packages/headless/src/index.ts b/packages/headless/src/index.ts index ec9d891f84..3581a1d385 100644 --- a/packages/headless/src/index.ts +++ b/packages/headless/src/index.ts @@ -45,7 +45,7 @@ import { import pkgIdToFilename from '@pnpm/pkgid-to-filename' import { readImporterManifestOnly } from '@pnpm/read-importer-manifest' import { fromDir as readPackageFromDir } from '@pnpm/read-package-json' -import { shamefullyFlattenByLockfile } from '@pnpm/shamefully-flatten' +import shamefullyFlatten from '@pnpm/shamefully-flatten' import { PackageFilesResponse, StoreController, @@ -228,27 +228,25 @@ export default async (opts: HeadlessOptions) => { }) } - await Promise.all(opts.importers.map(async (importer) => { - if (importer.shamefullyFlatten) { - importer.hoistedAliases = await shamefullyFlattenByLockfile(filteredLockfile, importer.id, { - getIndependentPackageLocation: opts.independentLeaves - ? async (packageId: string, packageName: string) => { - const { directory } = await opts.storeController.getPackageLocation(packageId, packageName, { - lockfileDirectory: opts.lockfileDirectory, - targetEngine: opts.sideEffectsCacheRead && ENGINE_NAME || undefined, - }) - return directory - } - : undefined, - lockfileDirectory: opts.lockfileDirectory, - modulesDir: importer.modulesDir, - registries: opts.registries, - virtualStoreDir, - }) - } else { - importer.hoistedAliases = {} - } - })) + const rootImporterWithFlatModules = opts.importers.find((importer) => importer.id === '.' && importer.shamefullyFlatten) + if (rootImporterWithFlatModules) { + rootImporterWithFlatModules.hoistedAliases = await shamefullyFlatten({ + getIndependentPackageLocation: opts.independentLeaves + ? async (packageId: string, packageName: string) => { + const { directory } = await opts.storeController.getPackageLocation(packageId, packageName, { + lockfileDirectory: opts.lockfileDirectory, + targetEngine: opts.sideEffectsCacheRead && ENGINE_NAME || undefined, + }) + return directory + } + : undefined, + lockfile: filteredLockfile, + lockfileDirectory: opts.lockfileDirectory, + modulesDir: rootImporterWithFlatModules.modulesDir, + registries: opts.registries, + virtualStoreDir, + }) + } await Promise.all(opts.importers.map(async ({ id, manifest, modulesDir, prefix }) => { await linkRootPackages(filteredLockfile, { diff --git a/packages/pnpm/src/cmd/recursive/index.ts b/packages/pnpm/src/cmd/recursive/index.ts index 6d03194090..9fd13e643d 100644 --- a/packages/pnpm/src/cmd/recursive/index.ts +++ b/packages/pnpm/src/cmd/recursive/index.ts @@ -225,6 +225,9 @@ export async function recursive ( if (cmdFullName !== 'rebuild') { // For a workspace with shared lockfile if (opts.lockfileDirectory && ['install', 'uninstall', 'update'].includes(cmdFullName)) { + if (opts.shamefullyFlatten) { + logger.info({ message: 'Only the root workspace package is going to have a flat node_modules', prefix: opts.lockfileDirectory }) + } let importers = await getImporters() const isFromWorkspace = isSubdir.bind(null, opts.lockfileDirectory) importers = await pFilter(importers, async ({ prefix }: { prefix: string }) => isFromWorkspace(await fs.realpath(prefix))) @@ -238,7 +241,7 @@ export async function recursive ( const { manifest, writeImporterManifest } = manifestsByPath[prefix] const shamefullyFlatten = typeof localConfigs.shamefullyFlatten === 'boolean' ? localConfigs.shamefullyFlatten - : opts.shamefullyFlatten + : (prefix === opts.lockfileDirectory && opts.shamefullyFlatten) let currentInput = [...input] if (updateToLatest) { if (!currentInput || !currentInput.length) { diff --git a/packages/pnpm/test/install/shamefullyFlatten.ts b/packages/pnpm/test/install/shamefullyFlatten.ts index 961b80780f..eecb181605 100644 --- a/packages/pnpm/test/install/shamefullyFlatten.ts +++ b/packages/pnpm/test/install/shamefullyFlatten.ts @@ -1,9 +1,12 @@ -import prepare from '@pnpm/prepare' +import prepare, { preparePackages } from '@pnpm/prepare' +import fs = require('mz/fs') import tape = require('tape') import promisifyTape from 'tape-promise' +import writeYamlFile = require('write-yaml-file') import { execPnpm } from '../utils' const test = promisifyTape(tape) +const testOnly = promisifyTape(tape.only) test('shamefully flatten the dependency tree', async function (t) { const project = prepare(t) @@ -20,3 +23,36 @@ test('shamefully flatten the dependency tree', async function (t) { await project.hasNot('debug') await project.hasNot('cookie') }) + +test('shamefully-flatten: applied only to the workspace root package when set to true in the root .npmrc file', async (t: tape.Test) => { + const projects = preparePackages(t, [ + { + location: '.', + package: { + name: 'root', + + dependencies: { + 'pkg-with-1-dep': '100.0.0', + }, + }, + }, + { + name: 'project', + version: '1.0.0', + + dependencies: { + 'foobar': '100.0.0', + }, + }, + ]) + + await writeYamlFile('pnpm-workspace.yaml', { packages: ['**', '!store/**'] }) + await fs.writeFile('.npmrc', 'shamefully-flatten', 'utf8') + + await execPnpm('recursive', 'install') + + await projects['root'].has('dep-of-pkg-with-1-dep') + await projects['root'].hasNot('foo') + await projects['project'].hasNot('foo') + await projects['project'].has('foobar') +}) diff --git a/packages/pnpm/test/monorepo/index.ts b/packages/pnpm/test/monorepo/index.ts index e71734de57..139c7e8ec2 100644 --- a/packages/pnpm/test/monorepo/index.ts +++ b/packages/pnpm/test/monorepo/index.ts @@ -423,7 +423,7 @@ test('recursive install with link-workspace-packages and shared-workspace-lockfi await writeYamlFile('pnpm-workspace.yaml', { packages: ['**', '!store/**'] }) await fs.writeFile( 'is-positive/.npmrc', - 'shamefully-flatten = true\nsave-exact = true', + 'save-exact = true', 'utf8', ) await fs.writeFile( @@ -435,7 +435,6 @@ test('recursive install with link-workspace-packages and shared-workspace-lockfi await execPnpm('recursive', 'install', '--link-workspace-packages', '--shared-workspace-lockfile=true', '--store', 'store') t.ok(projects['is-positive'].requireModule('is-negative')) - t.ok(projects['is-positive'].requireModule('concat-stream'), 'dependencies flattened in is-positive') t.notOk(projects['project-1'].requireModule('is-positive/package.json').author, 'local package is linked') const sharedLockfile = await readYamlFile(WANTED_LOCKFILE) diff --git a/packages/shamefully-flatten/src/index.ts b/packages/shamefully-flatten/src/index.ts index dfc7d981f7..f7bdb64693 100644 --- a/packages/shamefully-flatten/src/index.ts +++ b/packages/shamefully-flatten/src/index.ts @@ -13,20 +13,19 @@ import * as dp from 'dependency-path' import path = require('path') import R = require('ramda') -export async function shamefullyFlattenByLockfile ( - lockfile: Lockfile, - importerId: string, +export default async function shamefullyFlattenByLockfile ( opts: { getIndependentPackageLocation?: (packageId: string, packageName: string) => Promise, + lockfile: Lockfile, lockfileDirectory: string, modulesDir: string, registries: Registries, virtualStoreDir: string, }, ) { - if (!lockfile.packages) return {} + if (!opts.lockfile.packages) return {} - const lockfileImporter = lockfile.importers[importerId] + const lockfileImporter = opts.lockfile.importers['.'] const entryNodes = R.toPairs({ ...lockfileImporter.devDependencies, @@ -36,7 +35,7 @@ export async function shamefullyFlattenByLockfile ( .map((pair) => dp.refToRelative(pair[1], pair[0])) .filter((nodeId) => nodeId !== null) as string[] - const deps = await getDependencies(lockfile.packages, entryNodes, new Set(), 0, { + const deps = await getDependencies(opts.lockfile.packages, entryNodes, new Set(), 0, { getIndependentPackageLocation: opts.getIndependentPackageLocation, lockfileDirectory: opts.lockfileDirectory, registries: opts.registries, @@ -125,7 +124,7 @@ export interface Dependency { absolutePath: string, } -export default async function shamefullyFlattenGraph ( +async function shamefullyFlattenGraph ( depNodes: Dependency[], currentSpecifiers: {[alias: string]: string}, opts: { diff --git a/packages/supi/src/install/index.ts b/packages/supi/src/install/index.ts index a24857197d..a86559c53d 100644 --- a/packages/supi/src/install/index.ts +++ b/packages/supi/src/install/index.ts @@ -133,6 +133,17 @@ export async function install ( export type MutatedImporter = ImportersOptions & DependenciesMutation +export class ShamefullyFlattenNotInLockfileDirectoryError extends PnpmError { + public readonly shamefullyFlattenDirectory: string + public readonly lockfileDirectory: string + + constructor (shamefullyFlattenDirectory: string, lockfileDirectory: string) { + super('SHAMEFULLY_FLATTEN_NOT_IN_LOCKFILE_DIR', 'Shamefully flatten can be only used in the lockfile directory') + this.shamefullyFlattenDirectory = shamefullyFlattenDirectory + this.lockfileDirectory = lockfileDirectory + } +} + export async function mutateModules ( importers: MutatedImporter[], maybeOpts: InstallOptions & { @@ -154,6 +165,11 @@ export async function mutateModules ( if (!opts.include.dependencies && opts.include.optionalDependencies) { throw new PnpmError('OPTIONAL_DEPS_REQUIRE_PROD_DEPS', 'Optional dependencies cannot be installed without production dependencies') } + for (const { prefix, shamefullyFlatten } of importers) { + if (prefix !== opts.lockfileDirectory && shamefullyFlatten) { + throw new ShamefullyFlattenNotInLockfileDirectoryError(prefix, opts.lockfileDirectory) + } + } const ctx = await getContext(importers, opts) @@ -789,6 +805,7 @@ async function installInContext ( outdatedDependencies, pruneStore: opts.pruneStore, registries: ctx.registries, + sideEffectsCacheRead: opts.sideEffectsCacheRead, skipped: ctx.skipped, storeController: opts.storeController, strictPeerDependencies: opts.strictPeerDependencies, diff --git a/packages/supi/src/install/link.ts b/packages/supi/src/install/link.ts index 8a0475087e..b9c8128e97 100644 --- a/packages/supi/src/install/link.ts +++ b/packages/supi/src/install/link.ts @@ -1,4 +1,5 @@ import { + ENGINE_NAME, LOCKFILE_VERSION, WANTED_LOCKFILE, } from '@pnpm/constants' @@ -15,7 +16,7 @@ import logger from '@pnpm/logger' import { prune } from '@pnpm/modules-cleaner' import { IncludedDependencies } from '@pnpm/modules-yaml' import { DependenciesTree, LinkedDependency } from '@pnpm/resolve-dependencies' -import shamefullyFlattenGraph from '@pnpm/shamefully-flatten' +import shamefullyFlatten from '@pnpm/shamefully-flatten' import { StoreController } from '@pnpm/store-controller-types' import symlinkDependency, { symlinkDirectRootDependency } from '@pnpm/symlink-dependency' import { ImporterManifest, Registries } from '@pnpm/types' @@ -76,6 +77,7 @@ export default async function linkPackages ( updateLockfileMinorVersion: boolean, outdatedDependencies: {[pkgId: string]: string}, strictPeerDependencies: boolean, + sideEffectsCacheRead: boolean, }, ): Promise<{ currentLockfile: Lockfile, @@ -299,27 +301,26 @@ export default async function linkPackages ( currentLockfile = newCurrentLockfile } - // Important: shamefullyFlattenGraph changes depGraph, so keep this at the end, right before linkBins if (newDepPaths.length > 0 || removedDepPaths.size > 0) { - await Promise.all( - importers.filter(({ shamefullyFlatten }) => shamefullyFlatten) - .map(async (importer) => { - importer.hoistedAliases = await shamefullyFlattenGraph( - depNodes.map((depNode) => ({ - absolutePath: depNode.absolutePath, - children: depNode.children, - depth: depNode.depth, - location: depNode.independent ? depNode.centralLocation : depNode.peripheralLocation, - name: depNode.name, - })), - currentLockfile.importers[importer.id].specifiers, - { - dryRun: opts.dryRun, - modulesDir: importer.modulesDir, - }, - ) - }), - ) + const rootImporterWithFlatModules = importers.find((importer) => importer.id === '.' && importer.shamefullyFlatten) + if (rootImporterWithFlatModules) { + rootImporterWithFlatModules.hoistedAliases = await shamefullyFlatten({ + getIndependentPackageLocation: opts.independentLeaves + ? async (packageId: string, packageName: string) => { + const { directory } = await opts.storeController.getPackageLocation(packageId, packageName, { + lockfileDirectory: opts.lockfileDirectory, + targetEngine: opts.sideEffectsCacheRead && ENGINE_NAME || undefined, + }) + return directory + } + : undefined, + lockfile: currentLockfile, + lockfileDirectory: opts.lockfileDirectory, + modulesDir: rootImporterWithFlatModules.modulesDir, + registries: opts.registries, + virtualStoreDir: opts.virtualStoreDir, + }) + } } if (!opts.dryRun) { diff --git a/packages/supi/test/install/shamefullyFlatten.ts b/packages/supi/test/install/shamefullyFlatten.ts index d70fc8846a..e861fa8585 100644 --- a/packages/supi/test/install/shamefullyFlatten.ts +++ b/packages/supi/test/install/shamefullyFlatten.ts @@ -1,7 +1,15 @@ -import { prepareEmpty } from '@pnpm/prepare' +import { prepareEmpty, preparePackages } from '@pnpm/prepare' import fs = require('fs') +import path = require('path') import resolveLinkTarget = require('resolve-link-target') -import { addDependenciesToPackage, install, mutateModules } from 'supi' +import rimraf = require('rimraf-then') +import { + addDependenciesToPackage, + install, + MutatedImporter, + mutateModules, + ShamefullyFlattenNotInLockfileDirectoryError, +} from 'supi' import tape = require('tape') import promisifyTape from 'tape-promise' import { addDistTag, testDefaults } from '../utils' @@ -252,3 +260,90 @@ test('should uninstall correctly peer dependencies', async (t) => { t.throws(() => fs.lstatSync('node_modules/ajv-keywords'), Error, 'symlink to peer dependency is deleted') }) + +test('shamefully-flatten: throw exception when executed on a project that uses an external lockfile', async (t: tape.Test) => { + prepareEmpty(t) + const lockfileDirectory = path.resolve('..') + + let err!: ShamefullyFlattenNotInLockfileDirectoryError + try { + await addDependenciesToPackage({}, ['is-negative'], await testDefaults({ shamefullyFlatten: true, lockfileDirectory })) + t.fail('installation should have failed') + } catch (_err) { + err = _err + } + + t.ok(err, 'error thrown') + t.equal(err.code, 'ERR_PNPM_SHAMEFULLY_FLATTEN_NOT_IN_LOCKFILE_DIR') + t.equal(err.shamefullyFlattenDirectory, process.cwd()) + t.equal(err.lockfileDirectory, lockfileDirectory) +}) + +test('shamefully-flatten: only hoists the dependencies of the root workspace package', async (t) => { + const workspaceRootManifest = { + name: 'root', + + dependencies: { + 'pkg-with-1-dep': '100.0.0', + }, + } + const workspacePackageManifest = { + name: 'package', + + dependencies: { + 'foobar': '100.0.0' + }, + } + const projects = preparePackages(t, [ + { + location: '.', + package: workspaceRootManifest, + }, + { + location: 'package', + package: workspacePackageManifest, + }, + ]) + + const importers: MutatedImporter[] = [ + { + buildIndex: 0, + manifest: workspaceRootManifest, + mutation: 'install', + prefix: process.cwd(), + shamefullyFlatten: true, + }, + { + buildIndex: 0, + manifest: workspacePackageManifest, + mutation: 'install', + prefix: path.resolve('package'), + }, + ] + await mutateModules(importers, await testDefaults()) + + await projects['root'].has('pkg-with-1-dep') + await projects['root'].has('dep-of-pkg-with-1-dep') + await projects['root'].hasNot('foobar') + await projects['root'].hasNot('foo') + await projects['root'].hasNot('bar') + + await projects['package'].has('foobar') + await projects['package'].hasNot('foo') + await projects['package'].hasNot('bar') + + await rimraf('node_modules') + await rimraf('package/node_modules') + + await mutateModules(importers, await testDefaults({ frozenLockfile: true })) + + await projects['root'].has('pkg-with-1-dep') + await projects['root'].has('dep-of-pkg-with-1-dep') + await projects['root'].hasNot('foobar') + await projects['root'].hasNot('foo') + await projects['root'].hasNot('bar') + + await projects['package'].has('foobar') + await projects['package'].hasNot('foo') + await projects['package'].hasNot('bar') +})