From 72a7a5915e9d333d104d88bf73d7a555f9400e24 Mon Sep 17 00:00:00 2001
From: Gar <gar+gh@danger.computer>
Date: Thu, 26 Jan 2023 12:25:18 -0800
Subject: [PATCH] fix: only save package-lock when truly finished

When we update the trees, the edges are refreshed to correct their
specs.  If we have written the package-lock before this point, it will
be incorrect.

No new tests are needed as existing tests were snapshotted w/ the broken
behavior, and are fixed now showing this bugfix in action.
---
 workspaces/arborist/lib/arborist/reify.js     | 86 ++++++++-----------
 .../test/arborist/reify.js.test.cjs           |  6 +-
 2 files changed, 40 insertions(+), 52 deletions(-)

diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js
index 3c8059d281d1..87993cca876d 100644
--- a/workspaces/arborist/lib/arborist/reify.js
+++ b/workspaces/arborist/lib/arborist/reify.js
@@ -82,7 +82,6 @@ const _rollbackRetireShallowNodes = Symbol.for('rollbackRetireShallowNodes')
 const _rollbackCreateSparseTree = Symbol.for('rollbackCreateSparseTree')
 const _rollbackMoveBackRetiredUnchanged = Symbol.for('rollbackMoveBackRetiredUnchanged')
 const _saveIdealTree = Symbol.for('saveIdealTree')
-const _saveLockFile = Symbol('saveLockFile')
 const _copyIdealToActual = Symbol('copyIdealToActual')
 const _addOmitsToTrashList = Symbol('addOmitsToTrashList')
 const _packageLockOnly = Symbol('packageLockOnly')
@@ -1404,64 +1403,53 @@ module.exports = cls => class Reifier extends cls {
       }
     }
 
-    // preserve indentation, if possible
-    const {
-      [Symbol.for('indent')]: indent,
-    } = this.idealTree.package
-    const format = indent === undefined ? '  ' : indent
-
-    const saveOpt = {
-      format: (this[_formatPackageLock] && format) ? format
-      : this[_formatPackageLock],
-    }
-
-    const promises = [this[_saveLockFile](saveOpt)]
-
-    const updatePackageJson = async (tree) => {
-      const pkgJson = await PackageJson.load(tree.path)
-        .catch(() => new PackageJson(tree.path))
-      const {
-        dependencies = {},
-        devDependencies = {},
-        optionalDependencies = {},
-        peerDependencies = {},
-        // bundleDependencies is not required by PackageJson like the other fields here
-        // PackageJson also doesn't omit an empty array for this field so defaulting this
-        // to an empty array would add that field to every package.json file.
-        bundleDependencies,
-      } = tree.package
-
-      pkgJson.update({
-        dependencies,
-        devDependencies,
-        optionalDependencies,
-        peerDependencies,
-        bundleDependencies,
-      })
-      await pkgJson.save()
-    }
-
     if (save) {
       for (const tree of updatedTrees) {
         // refresh the edges so they have the correct specs
         tree.package = tree.package
-        promises.push(updatePackageJson(tree))
+        const pkgJson = await PackageJson.load(tree.path)
+          .catch(() => new PackageJson(tree.path))
+        const {
+          dependencies = {},
+          devDependencies = {},
+          optionalDependencies = {},
+          peerDependencies = {},
+          // bundleDependencies is not required by PackageJson like the other
+          // fields here PackageJson also doesn't omit an empty array for this
+          // field so defaulting this to an empty array would add that field to
+          // every package.json file.
+          bundleDependencies,
+        } = tree.package
+
+        pkgJson.update({
+          dependencies,
+          devDependencies,
+          optionalDependencies,
+          peerDependencies,
+          bundleDependencies,
+        })
+        await pkgJson.save()
       }
     }
 
-    await Promise.all(promises)
-    process.emit('timeEnd', 'reify:save')
-    return true
-  }
+    // before now edge specs could be changing, affecting the `requires` field
+    // in the package lock, so we hold off saving to the very last action
+    if (this[_usePackageLock]) {
+      // preserve indentation, if possible
+      let format = this.idealTree.package[Symbol.for('indent')]
+      if (format === undefined) {
+        format = '  '
+      }
 
-  async [_saveLockFile] (saveOpt) {
-    if (!this[_usePackageLock]) {
-      return
+      // TODO this ignores options.save
+      await this.idealTree.meta.save({
+        format: (this[_formatPackageLock] && format) ? format
+        : this[_formatPackageLock],
+      })
     }
 
-    const { meta } = this.idealTree
-
-    return meta.save(saveOpt)
+    process.emit('timeEnd', 'reify:save')
+    return true
   }
 
   async [_copyIdealToActual] () {
diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
index bfd9fa882147..2a92b735c3d0 100644
--- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
+++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
@@ -32675,7 +32675,7 @@ exports[`test/arborist/reify.js TAP save package.json on update should update na
     "a": {
       "version": "file:a",
       "requires": {
-        "abbrev": "^1.0.4",
+        "abbrev": "^1.1.1",
         "once": "^1.3.2"
       }
     },
@@ -32687,7 +32687,7 @@ exports[`test/arborist/reify.js TAP save package.json on update should update na
     "b": {
       "version": "file:b",
       "requires": {
-        "abbrev": "^1.0.4"
+        "abbrev": "^1.1.1"
       }
     },
     "once": {
@@ -32766,7 +32766,7 @@ exports[`test/arborist/reify.js TAP save package.json on update should update si
       "version": "file:a",
       "requires": {
         "abbrev": "^1.0.4",
-        "once": "^1.3.2"
+        "once": "^1.4.0"
       }
     },
     "abbrev": {