fix: stop optional dependencies from being added to prod inside lockfile

workspaces/arborist/lib/arborist/reify.js

@@ -1295,7 +1295,8 @@ module.exports = cls => class Reifier extends cls {
 
     const updatedTrees = new Set()
     const updateNodes = nodes => {
-      for (const { name, tree: addTree } of nodes) {
+      for (const node of nodes) {
+        const { name, tree: addTree } = node
         // addTree either the root, or a workspace
         const edge = addTree.edgesOut.get(name)
         const pkg = addTree.package
@@ -1371,15 +1372,25 @@ module.exports = cls => class Reifier extends cls {
           newSpec = req.saveSpec
         }
 
+        // delete optional dep from prod if installation doesn't fail
+        const shouldDeleteFromProd = pkg.optionalDependencies && pkg.optionalDependencies[name] && pkg.dependencies
+          && !this[_trashList].has(addTree.children.get(name).path)
+
         if (options.saveType) {
           const depType = saveTypeMap.get(options.saveType)
           pkg[depType][name] = newSpec
           // rpj will have moved it here if it was in both
           // if it is empty it will be deleted later
           if (options.saveType === 'prod' && pkg.optionalDependencies) {
             delete pkg.optionalDependencies[name]
+          } else if (options.saveType === 'optional' && shouldDeleteFromProd) {
+            delete pkg.dependencies[name]
           }
         } else {
+          if (shouldDeleteFromProd) {
+            delete pkg.dependencies[name]
+          }
+
           if (hasSubKey(pkg, 'dependencies', name)) {
             pkg.dependencies[name] = newSpec
           }

workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs

@@ -33087,6 +33087,55 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
 
 exports[`test/arborist/reify.js TAP save-prod, with optional > must match snapshot 1`] = `
 {"dependencies":{"abbrev":"^1.1.1"}}
+`
+exports[`test/arborist/reify.js TAP delete from prod, if optional dep is added > must match snapshot 1`] = `
+{
+  "name": "tap-testdir-reify-delete-from-prod-if-optional-dep-is-added",
+  "lockfileVersion": 3,
+  "requires": true,
+  "packages": {
+    "": {
+      "optionalDependencies": {
+        "abbrev": "^1.1.1"
+      }
+    },
+    "node_modules/abbrev": {
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
+      "license": "ISC",
+      "optional": true
+    }
+  }
+}
+
+`
+
+exports[`test/arborist/reify.js TAP keep in prod, if optional dep fails install > must match snapshot 1`] = `
+{
+  "name": "tap-testdir-reify-keep-in-prod-if-optional-dep-fails-install",
+  "lockfileVersion": 3,
+  "requires": true,
+  "packages": {
+    "": {
+      "dependencies": {
+        "@isaacs/testing-fail-install": "*"
+      },
+      "optionalDependencies": {
+        "@isaacs/testing-fail-install": "^1.0.0"
+      }
+    },
+    "node_modules/@isaacs/testing-fail-install": {
+      "version": "1.0.0",
+      "resolved": "https://registry.npmjs.org/@isaacs/testing-fail-install/-/testing-fail-install-1.0.0.tgz",
+      "integrity": "sha512-MlK05L4wasrk2/fQqgCeR8s+1KE8LEfMI3ZzaGfR5Dyvgi4R8vQY0fvwYcvKh6cqrmMX7wORvGKi5YJn8LZ9bg==",
+      "hasInstallScript": true,
+      "license": "ISC",
+      "optional": true
+    }
+  }
+}
+
 `
 
 exports[`test/arborist/reify.js TAP saveBundle > must match snapshot 1`] = `

workspaces/arborist/test/arborist/reify.js

@@ -1857,6 +1857,36 @@ t.test('save-prod, with optional', async t => {
   t.matchSnapshot(fs.readFileSync(path + '/package.json', 'utf8'))
 })
 
+t.test('delete from prod, if optional dep is added', async t => {
+  // add to existing optional dep in lockfile
+  const pathNoSaveType = t.testdir({
+    'package.json': JSON.stringify({
+      optionalDependencies: { abbrev: '*' },
+    }),
+  })
+  const arbNoSaveType = newArb({ path: pathNoSaveType })
+  await arbNoSaveType.reify({ add: ['abbrev'] })
+  const resultNoSaveType = fs.readFileSync(pathNoSaveType + '/package-lock.json', 'utf8')
+
+  // add to empty lockfile using optional saveType
+  const pathSaveType = t.testdir({ 'package.json': '{}' })
+  const arbSaveType = newArb({ path: pathSaveType })
+  await arbSaveType.reify({ add: ['abbrev'], saveType: 'optional' })
+  const resultSaveType = fs.readFileSync(pathSaveType + '/package-lock.json', 'utf8')
+
+  // both lockfiles should be the same
+  t.same(resultNoSaveType, resultSaveType)
+  t.matchSnapshot(resultNoSaveType)
+})
+
+t.test('keep in prod, if optional dep fails install', async t => {
+  const path = t.testdir({ 'package.json': '{}' })
+  const arb = newArb({ path })
+
+  await arb.reify({ add: ['@isaacs/testing-fail-install'], saveType: 'optional' })
+  t.matchSnapshot(fs.readFileSync(path + '/package-lock.json', 'utf8'))
+})
+
 t.test('saveBundle', async t => {
   const path = t.testdir({
     'package.json': JSON.stringify({