fix(ci): don't error on optional deps in the lockfile (#9083)

wraithgar · web-flow · commit 76c76e5ad7cc · 2026-03-09T10:08:58.000-07:00
We build a virtual tree from the lockfile before we build the ideal tree
so that we can compare the two and error if package.json differs from
what was used to build the lockfile.

The virtual tree includes optional deps in it that would otherwise be
ignored when building the ideal tree. In order not to have `npm ci` fail
in this situation we need to build the virtual tree separately, and
start clean when building the ideal tree.

This also brings in a few linting fixups.
diff --git a/lib/commands/ci.js b/lib/commands/ci.js
@@ -36,6 +36,8 @@ class CI extends ArboristWorkspaceCmd {
       })
     }
 
+    const dryRun = this.npm.config.get('dry-run')
+    const ignoreScripts = this.npm.config.get('ignore-scripts')
     const where = this.npm.prefix
     const Arborist = require('@npmcli/arborist')
     const opts = {
@@ -46,38 +48,35 @@ class CI extends ArboristWorkspaceCmd {
       workspaces: this.workspaceNames,
     }
 
-    const arb = new Arborist(opts)
-    await arb.loadVirtual().catch(er => {
-      log.verbose('loadVirtual', er.stack)
+    // generate an inventory from the virtual tree in the lockfile
+    const virtualArb = new Arborist(opts)
+    try {
+      await virtualArb.loadVirtual()
+    } catch (err) {
+      log.verbose('loadVirtual', err.stack)
       const msg =
         'The `npm ci` command can only install with an existing package-lock.json or\n' +
         'npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or\n' +
         'later to generate a package-lock.json file, then try again.'
       throw this.usageError(msg)
-    })
-
-    // retrieves inventory of packages from loaded virtual tree (lock file)
-    const virtualInventory = new Map(arb.virtualTree.inventory)
+    }
+    const virtualInventory = new Map(virtualArb.virtualTree.inventory)
 
-    // build ideal tree step needs to come right after retrieving the virtual
-    // inventory since it's going to erase the previous ref to virtualTree
+    // Now we make our real Arborist.
+    // We need a new one because the virtual tree fromt the lockfile can have extraneous dependencies in it that won't install on this platform
+    const arb = new Arborist(opts)
     await arb.buildIdealTree()
 
-    // verifies that the packages from the ideal tree will match
-    // the same versions that are present in the virtual tree (lock file)
-    // throws a validation error in case of mismatches
+    // Verifies that the packages from the ideal tree will match the same versions that are present in the virtual tree (lock file).
     const errors = validateLockfile(virtualInventory, arb.idealTree.inventory)
     if (errors.length) {
       throw this.usageError(
-        '`npm ci` can only install packages when your package.json and ' +
-        'package-lock.json or npm-shrinkwrap.json are in sync. Please ' +
-        'update your lock file with `npm install` ' +
-        'before continuing.\n\n' +
+        '`npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. ' +
+        'Please update your lock file with `npm install` before continuing.\n\n' +
         errors.join('\n')
       )
     }
 
-    const dryRun = this.npm.config.get('dry-run')
     if (!dryRun) {
       const workspacePaths = await getWorkspaces([], {
         path: this.npm.localPrefix,
@@ -100,7 +99,6 @@ class CI extends ArboristWorkspaceCmd {
 
     await arb.reify(opts)
 
-    const ignoreScripts = this.npm.config.get('ignore-scripts')
     // run the same set of scripts that `npm install` runs.
     if (!ignoreScripts) {
       const scripts = [
diff --git a/lib/utils/audit-error.js b/lib/utils/audit-error.js
@@ -7,7 +7,7 @@ const { redactLog: replaceInfo } = require('@npmcli/redact')
 // returns 'true' if there was an error, false otherwise
 
 const auditError = (npm, report) => {
-  if (!report || !report.error) {
+  if (!report?.error) {
     return false
   }
 
@@ -34,6 +34,7 @@ const auditError = (npm, report) => {
     output.standard(body)
   }
 
+  // XXX we should throw a real error here
   throw 'audit endpoint returned an error'
 }
 
diff --git a/lib/utils/reify-finish.js b/lib/utils/reify-finish.js
@@ -4,30 +4,18 @@ const { writeFile } = require('node:fs/promises')
 const { resolve } = require('node:path')
 
 const reifyFinish = async (npm, arb) => {
-  await saveBuiltinConfig(npm, arb)
-  reifyOutput(npm, arb)
-}
-
-const saveBuiltinConfig = async (npm, arb) => {
-  const { options: { global }, actualTree } = arb
-  if (!global) {
-    return
-  }
-
-  // if we are using a builtin config, and just installed npm as
-  // a top-level global package, we have to preserve that config.
-  const npmNode = actualTree.inventory.get('node_modules/npm')
-  if (!npmNode) {
-    return
+  // if we are using a builtin config, and just installed npm as a top-level global package, we have to preserve that config.
+  if (arb.options.global) {
+    const npmNode = arb.actualTree.inventory.get('node_modules/npm')
+    if (npmNode) {
+      const builtinConf = npm.config.data.get('builtin')
+      if (!builtinConf.loadError) {
+        const content = ini.stringify(builtinConf.raw).trim() + '\n'
+        await writeFile(resolve(npmNode.path, 'npmrc'), content)
+      }
+    }
   }
-
-  const builtinConf = npm.config.data.get('builtin')
-  if (builtinConf.loadError) {
-    return
-  }
-
-  const content = ini.stringify(builtinConf.raw).trim() + '\n'
-  await writeFile(resolve(npmNode.path, 'npmrc'), content)
+  reifyOutput(npm, arb)
 }
 
 module.exports = reifyFinish
diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js
@@ -20,9 +20,7 @@ const auditError = require('./audit-error.js')
 const reifyOutput = (npm, arb) => {
   const { diff, actualTree } = arb
 
-  // note: fails and crashes if we're running audit fix and there was an error
-  // which is a good thing, because there's no point printing all this other
-  // stuff in that case!
+  // note: fails and crashes if we're running audit fix and there was an error which is a good thing, because there's no point printing all this other stuff in that case!
   const auditReport = auditError(npm, arb.auditReport) ? null : arb.auditReport
 
   // don't print any info in --silent mode, but we still need to

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@ const { redactLog: replaceInfo } = require('@npmcli/redact')`
`7`	`7`	`// returns 'true' if there was an error, false otherwise`
`8`	`8`
`9`	`9`	`const auditError = (npm, report) => {`
`10`		`- if (!report \|\| !report.error) {`
	`10`	`+ if (!report?.error) {`
`11`	`11`	`return false`
`12`	`12`	`}`
`13`	`13`
`@@ -34,6 +34,7 @@ const auditError = (npm, report) => {`
`34`	`34`	`output.standard(body)`
`35`	`35`	`}`
`36`	`36`
	`37`	`+ // XXX we should throw a real error here`
`37`	`38`	`throw 'audit endpoint returned an error'`
`38`	`39`	`}`
`39`	`40`