fix(libnpmexec): fix read mixed local/registry pkg

Refactor / clean up of the logic around reading installed
packages. Fixes reading packages from mixed sources (one being
from the local installed tree and the other from the registry
using pacote.manifest).

Makes it so that libnpmexec is always reading from the
Arborist actual tree instead of reading `node_modules` from
the file system when retrieving local package data.

Fixes: https://github.com/npm/cli/issues/3668
Relates to: https://github.com/npm/cli/pull/4643
Relates to: https://github.com/npm/cli/issues/4619
Relates to: https://github.com/npm/statusboard/issues/403

3 files changed, 47 insertions, 77 deletions

diff --git a/workspaces/libnpmexec/lib/index.js b/workspaces/libnpmexec/lib/index.js
index 81d152a20..fbe5c5520 100644
--- a/workspaces/libnpmexec/lib/index.js
+++ b/workspaces/libnpmexec/lib/index.js
@@ -9,15 +9,14 @@ const npmlog = require('npmlog')
 const mkdirp = require('mkdirp-infer-owner')
 const npa = require('npm-package-arg')
 const pacote = require('pacote')
-const readPackageJson = require('read-package-json-fast')
 
 const cacheInstallDir = require('./cache-install-dir.js')
 const { fileExists, localFileExists } = require('./file-exists.js')
 const getBinFromManifest = require('./get-bin-from-manifest.js')
-const manifestMissing = require('./manifest-missing.js')
 const noTTY = require('./no-tty.js')
 const runScript = require('./run-script.js')
 const isWindows = require('./is-windows.js')
+const _localManifest = Symbol('localManifest')
 
 /* istanbul ignore next */
 const PATH = (
@@ -86,20 +85,42 @@ const exec = async (opts) => {
     packages.push(args[0])
   }
 
+  // figure out whether we need to install stuff, or if local is fine
+  const localArb = new Arborist({
+    ...flatOptions,
+    path,
+  })
+  const localTree = await localArb.loadActual()
+
+  const getLocalManifest = ({ tree, name }) => {
+    // look up the package name in the current tree inventory,
+    // if it's found then return that normalized pkg data
+    const [node] = tree.inventory.query('packageName', name)
+
+    if (node) {
+      return {
+        _id: node.pkgid,
+        ...node.package,
+        [_localManifest]: true,
+      }
+    }
+  }
+
   // If we do `npm exec foo`, and have a `foo` locally, then we'll
   // always use that, so we don't really need to fetch the manifest.
   // So: run npa on each packages entry, and if it is a name with a
-  // rawSpec==='', then try to readPackageJson at
-  // node_modules/${name}/package.json, and only pacote fetch if
-  // that fails.
+  // rawSpec==='', then try to find that node name in the tree inventory
+  // and only pacote fetch if that fails.
   const manis = await Promise.all(packages.map(async p => {
     const spec = npa(p, path)
     if (spec.type === 'tag' && spec.rawSpec === '') {
-      // fall through to the pacote.manifest() approach
-      try {
-        const pj = resolve(path, 'node_modules', spec.name, 'package.json')
-        return await readPackageJson(pj)
-      } catch (er) {}
+      const localManifest = getLocalManifest({
+        tree: localTree,
+        name: spec.name,
+      })
+      if (localManifest) {
+        return localManifest
+      }
     }
     // Force preferOnline to true so we are making sure to pull in the latest
     // This is especially useful if the user didn't give us a version, and
@@ -114,16 +135,9 @@ const exec = async (opts) => {
     args[0] = getBinFromManifest(manis[0])
   }
 
-  // figure out whether we need to install stuff, or if local is fine
-  const localArb = new Arborist({
-    ...flatOptions,
-    path,
-  })
-  const localTree = await localArb.loadActual()
-
-  // do we have all the packages in manifest list?
+  // are all packages from the manifest list installed?
   const needInstall =
-    manis.some(manifest => manifestMissing({ tree: localTree, manifest }))
+    manis.some(manifest => !manifest[_localManifest])
 
   if (needInstall) {
     const { npxCache } = flatOptions
@@ -135,16 +149,23 @@ const exec = async (opts) => {
     })
     const tree = await arb.loadActual()
 
+    // inspect the npx-space installed tree to check if the package is already
+    // there, if that's the case also check that it's version matches the same
+    // version expected by the user requested pkg returned by pacote.manifest
+    const filterMissingPackagesFromInstallDir = (mani) => {
+      const localManifest = getLocalManifest({ tree, name: mani.name })
+      if (localManifest) {
+        return localManifest.version !== mani.version
+      }
+      return true
+    }
+
     // at this point, we have to ensure that we get the exact same
     // version, because it's something that has only ever been installed
     // by npm exec in the cache install directory
-    const add = manis.filter(mani => manifestMissing({
-      tree,
-      manifest: {
-        ...mani,
-        _from: `${mani.name}@${mani.version}`,
-      },
-    }))
+    const add = manis
+      .filter(mani => !mani[_localManifest])
+      .filter(filterMissingPackagesFromInstallDir)
       .map(mani => mani._from)
       .sort((a, b) => a.localeCompare(b, 'en'))
 
diff --git a/workspaces/libnpmexec/lib/manifest-missing.js b/workspaces/libnpmexec/lib/manifest-missing.js
deleted file mode 100644
index aec1281e3..000000000
--- a/workspaces/libnpmexec/lib/manifest-missing.js
+++ /dev/null
@@ -1,19 +0,0 @@
-const manifestMissing = ({ tree, manifest }) => {
-  // if the tree doesn't have a child by that name/version, return true
-  // true means we need to install it
-  const child = tree.children.get(manifest.name)
-  // if no child, we have to load it
-  if (!child) {
-    return true
-  }
-
-  // if no version/tag specified, allow whatever's there
-  if (manifest._from === `${manifest.name}@`) {
-    return false
-  }
-
-  // otherwise the version has to match what we WOULD get
-  return child.version !== manifest.version
-}
-
-module.exports = manifestMissing
diff --git a/workspaces/libnpmexec/test/manifest-missing.js b/workspaces/libnpmexec/test/manifest-missing.js
deleted file mode 100644
index e7ce1c851..000000000
--- a/workspaces/libnpmexec/test/manifest-missing.js
+++ /dev/null
@@ -1,32 +0,0 @@
-const t = require('tap')
-const Arborist = require('@npmcli/arborist')
-
-const manifestMissing = require('../lib/manifest-missing.js')
-
-t.test('missing version', async t => {
-  const path = t.testdir({
-    node_modules: {
-      a: {
-        'package.json': JSON.stringify({
-          name: 'a',
-          version: '1.0.0',
-        }),
-      },
-    },
-    'package.json': JSON.stringify({
-      name: 'root',
-      dependencies: {
-        a: '^1.0.0',
-      },
-    }),
-  })
-  const arb = new Arborist({
-    path,
-  })
-  const tree = await arb.loadActual()
-  const manifest = {
-    name: 'a',
-    _from: 'a@',
-  }
-  t.notOk(manifestMissing({ tree, manifest }), 'manifest not missing')
-})