From ea294924462dd3b79bafcdc69a8121bbd7ba8341 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 13 Apr 2019 14:42:41 +0300 Subject: [PATCH] fix: a non-optional package's install script fails When a non-optional package is not built successfully, the package.json, pnpm-lock.yaml and node_modules/.pnpm-lock.yaml should not be updated. close #1397 --- packages/headless/src/index.ts | 48 ++++++------ packages/supi/src/install/index.ts | 76 +++++++++++-------- packages/supi/src/install/link.ts | 1 + packages/supi/src/save.ts | 7 +- .../supi/test/install/lifecycleScripts.ts | 59 ++++++++++++++ 5 files changed, 134 insertions(+), 57 deletions(-) diff --git a/packages/headless/src/index.ts b/packages/headless/src/index.ts index 1ab6544eb2..cc0bc9f7d5 100644 --- a/packages/headless/src/index.ts +++ b/packages/headless/src/index.ts @@ -260,11 +260,6 @@ export default async (opts: HeadlessOptions) => { }) })) - if (currentLockfile && !R.equals(opts.importers.map((importer) => importer.id).sort(), Object.keys(filteredLockfile.importers).sort())) { - Object.assign(filteredLockfile.packages, currentLockfile.packages) - } - await writeCurrentLockfile(lockfileDirectory, filteredLockfile) - if (opts.ignoreScripts) { for (const importer of opts.importers) { if (opts.ignoreScripts && importer.pkg && importer.pkg.scripts && @@ -283,26 +278,7 @@ export default async (opts: HeadlessOptions) => { .filter((node) => node.requiresBuild) .map((node) => node.relDepPath), ) - } - await writeModulesYaml(virtualStoreDir, { - importers: opts.importers.reduce((acc, importer) => { - acc[importer.id] = { - hoistedAliases: importer.hoistedAliases, - shamefullyFlatten: importer.shamefullyFlatten, - } - return acc - }, {}), - included: opts.include, - independentLeaves: !!opts.independentLeaves, - layoutVersion: LAYOUT_VERSION, - packageManager: `${opts.packageManager.name}@${opts.packageManager.version}`, - pendingBuilds: opts.pendingBuilds, - registries: opts.registries, - skipped: Array.from(skipped), - store: opts.store, - }) - - if (!opts.ignoreScripts) { + } else { const directNodes = new Set() for (const importer of opts.importers) { R @@ -328,6 +304,28 @@ export default async (opts: HeadlessOptions) => { await linkAllBins(depGraph, { optional: opts.include.optionalDependencies, warn }) await Promise.all(opts.importers.map(linkBinsOfImporter)) + if (currentLockfile && !R.equals(opts.importers.map((importer) => importer.id).sort(), Object.keys(filteredLockfile.importers).sort())) { + Object.assign(filteredLockfile.packages, currentLockfile.packages) + } + await writeCurrentLockfile(lockfileDirectory, filteredLockfile) + await writeModulesYaml(virtualStoreDir, { + importers: opts.importers.reduce((acc, importer) => { + acc[importer.id] = { + hoistedAliases: importer.hoistedAliases, + shamefullyFlatten: importer.shamefullyFlatten, + } + return acc + }, {}), + included: opts.include, + independentLeaves: !!opts.independentLeaves, + layoutVersion: LAYOUT_VERSION, + packageManager: `${opts.packageManager.name}@${opts.packageManager.version}`, + pendingBuilds: opts.pendingBuilds, + registries: opts.registries, + skipped: Array.from(skipped), + store: opts.store, + }) + // waiting till package requests are finished await Promise.all(R.values(depGraph).map((depNode) => depNode.finishing)) diff --git a/packages/supi/src/install/index.ts b/packages/supi/src/install/index.ts index 058086ca31..c188137ad1 100644 --- a/packages/supi/src/install/index.ts +++ b/packages/supi/src/install/index.ts @@ -54,6 +54,7 @@ import path = require('path') import R = require('ramda') import rimraf = require('rimraf-then') import semver = require('semver') +import writePkg = require('write-pkg') import { PnpmError } from '../errorTypes' import getContext, { ImportersOptions, PnpmContext } from '../getContext' import getSpecFromPackageJson from '../getSpecFromPackageJson' @@ -696,10 +697,12 @@ async function installInContext ( const importersToLink = await Promise.all(importers.map(async (importer) => { const resolvedImporter = resolvedImporters[importer.id] let newPkg: PackageJson | undefined = importer.pkg + let writePackageJson!: boolean if (importer.updatePackageJson && importer.mutation === 'installSome') { if (!importer.pkg) { throw new Error('Cannot save because no package.json found') } + writePackageJson = true const specsToUsert = resolvedImporter.directDependencies // tslint:disable-line .filter((dep) => importer.newPkgRawSpecs.includes(dep.specRaw)) .map((dep) => { @@ -723,8 +726,10 @@ async function installInContext ( newPkg = await save( importer.prefix, specsToUsert, + { dryRun: true }, ) } else { + writePackageJson = false packageJsonLogger.debug({ prefix: importer.prefix, updated: importer.pkg, @@ -768,6 +773,7 @@ async function installInContext ( shamefullyFlatten: importer.shamefullyFlatten, topParents, usesExternalLockfile: importer.usesExternalLockfile, + writePackageJson, } })) @@ -809,6 +815,45 @@ async function installInContext ( ) } + if (!opts.lockfileOnly) { + // postinstall hooks + if (!opts.ignoreScripts && result.newDepPaths && result.newDepPaths.length) { + const depPaths = Object.keys(result.depGraph) + const rootNodes = depPaths.filter((depPath) => result.depGraph[depPath].depth === 0) + + await buildModules(result.depGraph, rootNodes, { + childConcurrency: opts.childConcurrency, + depsToBuild: new Set(result.newDepPaths), + optional: opts.include.optionalDependencies, + prefix: ctx.lockfileDirectory, + rawNpmConfig: opts.rawNpmConfig, + rootNodeModulesDir: ctx.virtualStoreDir, + sideEffectsCacheWrite: opts.sideEffectsCacheWrite, + storeController: opts.storeController, + unsafePerm: opts.unsafePerm, + userAgent: opts.userAgent, + }) + } + + if (result.newDepPaths && result.newDepPaths.length) { + const newPkgs = R.props(result.newDepPaths, result.depGraph) + await linkAllBins(newPkgs, result.depGraph, { + optional: opts.include.optionalDependencies, + warn: (message: string) => logger.warn({ message, prefix: opts.lockfileDirectory }), + }) + } + + if (!opts.lockfileOnly) { + await Promise.all(importersToLink.map(linkBinsOfImporter)) + } + } + + await Promise.all( + importersToLink + .filter((importer) => importer.writePackageJson) + .map((importer) => writePkg(path.join(importer.prefix, 'package.json'), importer.pkg)) + ) + const lockfileOpts = { forceSharedFormat: opts.forceSharedLockfile } if (opts.lockfileOnly) { await writeWantedLockfile(ctx.lockfileDirectory, result.wantedLockfile, lockfileOpts) @@ -844,37 +889,6 @@ async function installInContext ( }) })(), ]) - - // postinstall hooks - if (!opts.ignoreScripts && result.newDepPaths && result.newDepPaths.length) { - const depPaths = Object.keys(result.depGraph) - const rootNodes = depPaths.filter((depPath) => result.depGraph[depPath].depth === 0) - - await buildModules(result.depGraph, rootNodes, { - childConcurrency: opts.childConcurrency, - depsToBuild: new Set(result.newDepPaths), - optional: opts.include.optionalDependencies, - prefix: ctx.lockfileDirectory, - rawNpmConfig: opts.rawNpmConfig, - rootNodeModulesDir: ctx.virtualStoreDir, - sideEffectsCacheWrite: opts.sideEffectsCacheWrite, - storeController: opts.storeController, - unsafePerm: opts.unsafePerm, - userAgent: opts.userAgent, - }) - } - - if (result.newDepPaths && result.newDepPaths.length) { - const newPkgs = R.props(result.newDepPaths, result.depGraph) - await linkAllBins(newPkgs, result.depGraph, { - optional: opts.include.optionalDependencies, - warn: (message: string) => logger.warn({ message, prefix: opts.lockfileDirectory }), - }) - } - - if (!opts.lockfileOnly) { - await Promise.all(importersToLink.map(linkBinsOfImporter)) - } } // waiting till the skipped packages are downloaded to the store diff --git a/packages/supi/src/install/link.ts b/packages/supi/src/install/link.ts index c9f33a8b48..dda6ae061f 100644 --- a/packages/supi/src/install/link.ts +++ b/packages/supi/src/install/link.ts @@ -52,6 +52,7 @@ export interface Importer { shamefullyFlatten: boolean, topParents: Array<{name: string, version: string}>, usesExternalLockfile: boolean, + writePackageJson: boolean, } export default async function linkPackages ( diff --git a/packages/supi/src/save.ts b/packages/supi/src/save.ts index 8a1e3bc26e..77ca63c61f 100644 --- a/packages/supi/src/save.ts +++ b/packages/supi/src/save.ts @@ -15,6 +15,9 @@ export default async function save ( pref?: string, saveType?: DependenciesField, }>, + opts?: { + dryRun?: boolean, + } ): Promise { // Read the latest version of package.json to avoid accidental overwriting let packageJson: object @@ -42,7 +45,9 @@ export default async function save ( } }) - await writePkg(pkgJsonPath, packageJson) + if (!opts || opts.dryRun !== true) { + await writePkg(pkgJsonPath, packageJson) + } packageJsonLogger.debug({ prefix, updated: packageJson, diff --git a/packages/supi/test/install/lifecycleScripts.ts b/packages/supi/test/install/lifecycleScripts.ts index afa4e9bbaf..609a4b675a 100644 --- a/packages/supi/test/install/lifecycleScripts.ts +++ b/packages/supi/test/install/lifecycleScripts.ts @@ -343,3 +343,62 @@ test('bins are linked even if lifecycle scripts are ignored', async (t: tape.Tes t.ok(await exists('node_modules/pre-and-postinstall-scripts-example/package.json')) t.notOk(await exists('node_modules/pre-and-postinstall-scripts-example/generated-by-preinstall.js'), 'scripts were ignored indeed') }) + +test('dependency should not be added to package.json and lockfile if it was not built successfully', async (t: tape.Test) => { + const project = prepare(t) + const initialPkgJson = await loadJsonFile(path.resolve('package.json')) + + let err + try { + await addDependenciesToPackage( + [ + 'package-that-cannot-be-installed@0.0.0', + ], + await testDefaults(), + ) + } catch (_err) { + err = _err + } + + t.ok(err) + + t.notOk(await project.loadCurrentLockfile()) + t.notOk(await project.loadLockfile()) + + const pkg = await loadJsonFile(path.resolve('package.json')) + t.deepEqual(pkg, initialPkgJson, 'package.json not updated') +}) + +test('dependency should not be added current lockfile if it was not built successfully during headless install', async (t: tape.Test) => { + const project = prepare(t) + + await addDependenciesToPackage( + [ + 'package-that-cannot-be-installed@0.0.0', + ], + await testDefaults({ + ignoreScripts: true, + lockfileOnly: true, + }), + ) + + let err + try { + await mutateModules( + [ + { + buildIndex: 0, + mutation: 'install', + prefix: process.cwd(), + }, + ], + await testDefaults({ frozenLockfile: true }), + ) + } catch (_err) { + err = _err + } + + t.ok(err) + + t.notOk(await project.loadCurrentLockfile()) +})