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

@@ -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<string>()
     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))
 

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<ImporterToLink>(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 = <any>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<string, DependenciesGraphNode>(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<string, DependenciesGraphNode>(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

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 (

packages/supi/src/save.ts

@@ -15,6 +15,9 @@ export default async function save (
     pref?: string,
     saveType?: DependenciesField,
   }>,
+  opts?: {
+    dryRun?: boolean,
+  }
 ): Promise<PackageJson> {
   // 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,

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())
+})