From 64ab520833f707eea8ffc20180c305568dc3c8f2 Mon Sep 17 00:00:00 2001
From: Gar <gar+gh@danger.computer>
Date: Wed, 8 May 2024 09:57:59 -0700
Subject: [PATCH] fix: avoid caching manifests as promises

Originally this was in https://github.com/npm/cli/pull/7468:

We backed off of it while we were rebuilding pacote's packument cache.

Now that that's done we can assess this in isolation.  I think it makes
sense.  The packument is cached here, all this is awaiting is
normalization and ssri calculation.

The only place this potentially does anything is in the premature
loading of manifests in `#buildDepStep` when looking at problem edges.
We can just wait till we need them and not throw a ton of requests in
parallel before we actually need them.

Removing the premature loading in problem edges will have to be a
separate effort, as it is somehow load bearing
---
 .../arborist/lib/arborist/build-ideal-tree.js | 22 ++++++++-----------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js
index 3a35810a76e83..d0d159976c5db 100644
--- a/workspaces/arborist/lib/arborist/build-ideal-tree.js
+++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js
@@ -1022,6 +1022,10 @@ This is a one-time fix-up, please be patient...
           // may well be an optional dep that has gone missing.  it'll
           // fail later anyway.
           for (const e of this.#problemEdges(placed)) {
+            // XXX This is somehow load bearing.  This makes tests that print
+            // the ideal tree of a tree with tarball dependencies fail. This
+            // can't be changed or removed till we figure out why
+            // The test is named "tarball deps with transitive tarball deps"
             promises.push(() =>
               this.#fetchManifest(npa.resolve(e.name, e.spec, fromPath(placed, e)))
                 .catch(() => null)
@@ -1204,6 +1208,7 @@ This is a one-time fix-up, please be patient...
     const options = {
       ...this.options,
       avoid: this.#avoidRange(spec.name),
+      fullMetadata: true,
     }
     // get the intended spec and stored metadata from yarn.lock file,
     // if available and valid.
@@ -1212,19 +1217,10 @@ This is a one-time fix-up, please be patient...
     if (this.#manifests.has(spec.raw)) {
       return this.#manifests.get(spec.raw)
     } else {
-      const cleanRawSpec = redact(spec.rawSpec)
-      log.silly('fetch manifest', spec.raw.replace(spec.rawSpec, cleanRawSpec))
-      const o = {
-        ...options,
-        fullMetadata: true,
-      }
-      const p = pacote.manifest(spec, o)
-        .then((mani) => {
-          this.#manifests.set(spec.raw, mani)
-          return mani
-        })
-      this.#manifests.set(spec.raw, p)
-      return p
+      log.silly('fetch manifest', spec.raw.replace(spec.rawSpec, redact(spec.rawSpec)))
+      const mani = await pacote.manifest(spec, options)
+      this.#manifests.set(spec.raw, mani)
+      return mani
     }
   }