fix(npx): link and run own bins directly when possible

DEPENDENCIES.md

@@ -34,6 +34,7 @@ graph LR;
   libnpmexec-->npmcli-arborist["@npmcli/arborist"];
   libnpmexec-->npmcli-ci-detect["@npmcli/ci-detect"];
   libnpmexec-->npmcli-eslint-config["@npmcli/eslint-config"];
+  libnpmexec-->npmcli-mock-registry["@npmcli/mock-registry"];
   libnpmexec-->npmcli-run-script["@npmcli/run-script"];
   libnpmexec-->npmcli-template-oss["@npmcli/template-oss"];
   libnpmexec-->npmlog;
@@ -353,12 +354,15 @@ graph LR;
   libnpmdiff-->tar;
   libnpmexec-->bin-links;
   libnpmexec-->chalk;
+  libnpmexec-->just-extend;
+  libnpmexec-->just-safe-set;
   libnpmexec-->minify-registry-metadata;
   libnpmexec-->mkdirp;
   libnpmexec-->npm-package-arg;
   libnpmexec-->npmcli-arborist["@npmcli/arborist"];
   libnpmexec-->npmcli-ci-detect["@npmcli/ci-detect"];
   libnpmexec-->npmcli-eslint-config["@npmcli/eslint-config"];
+  libnpmexec-->npmcli-mock-registry["@npmcli/mock-registry"];
   libnpmexec-->npmcli-run-script["@npmcli/run-script"];
   libnpmexec-->npmcli-template-oss["@npmcli/template-oss"];
   libnpmexec-->npmlog;
@@ -774,8 +778,8 @@ Each group depends on packages lower down the chain, nothing depends on
 packages higher up the chain.
 
  - npm
- - @npmcli/smoke-tests, libnpmpublish
- - @npmcli/mock-registry, libnpmdiff, libnpmexec, libnpmfund, libnpmpack
+ - @npmcli/smoke-tests, libnpmexec, libnpmpublish
+ - @npmcli/mock-registry, libnpmdiff, libnpmfund, libnpmpack
  - @npmcli/arborist
  - @npmcli/metavuln-calculator
  - pacote, libnpmaccess, libnpmhook, libnpmorg, libnpmsearch, libnpmteam, npm-profile

mock-registry/lib/index.js

@@ -10,6 +10,7 @@ class MockRegistry {
   #authorization
   #basic
   #debug
+  #strict
 
   constructor (opts) {
     if (!opts.registry) {
@@ -19,20 +20,48 @@ class MockRegistry {
     this.#authorization = opts.authorization
     this.#basic = opts.basic
     this.#debug = opts.debug
+    this.#strict = opts.strict
     // Required for this.package
     this.#tap = opts.tap
+    if (this.#tap) {
+      this.startNock()
+    }
   }
 
-  static tnock (t, host, opts, { debug = false } = {}) {
-    if (debug) {
-      Nock.emitter.on('no match', req => console.error('NO MATCH', req.options))
+  static tnock (t, host, opts, { debug = false, strict = false } = {}) {
+    const noMatch = (req) => {
+      if (strict) {
+        // There are network requests that get caught regardless of error code.
+        // Turning on strict mode requires that those requests get explicitly
+        // mocked with a 404, 500, etc.
+        // XXX: this is opt-in currently because it breaks some existing CLI
+        // tests. We should work towards making this the default for all tests.
+        t.fail(`Unmatched request: ${JSON.stringify(req.options, null, 2)}`)
+      }
+      if (debug) {
+        console.error('NO MATCH', t.name, req.options)
+      }
     }
+
+    Nock.emitter.on('no match', noMatch)
     Nock.disableNetConnect()
     const server = Nock(host, opts)
+
+    if (strict) {
+      // this requires that mocks not be shared between sub tests but it helps
+      // find mistakes quicker instead of waiting for the entire test to end
+      t.afterEach((t) => {
+        t.strictSame(server.pendingMocks(), [], 'no pending mocks after each')
+        t.strictSame(server.activeMocks(), [], 'no active mocks after each')
+      })
+    }
+
     t.teardown(() => {
       Nock.enableNetConnect()
       server.done()
+      Nock.emitter.off('no match', noMatch)
     })
+
     return server
   }
 
@@ -41,31 +70,38 @@ class MockRegistry {
   }
 
   get nock () {
-    if (!this.#nock) {
-      if (!this.#tap) {
-        throw new Error('cannot mock packages without a tap fixture')
-      }
-      const reqheaders = {}
-      if (this.#authorization) {
-        reqheaders.authorization = `Bearer ${this.#authorization}`
-      }
-      if (this.#basic) {
-        reqheaders.authorization = `Basic ${this.#basic}`
-      }
-      this.#nock = MockRegistry.tnock(
-        this.#tap,
-        this.#registry,
-        { reqheaders },
-        { debug: this.#debug }
-      )
-    }
     return this.#nock
   }
 
   set nock (nock) {
     this.#nock = nock
   }
 
+  startNock () {
+    if (this.nock) {
+      return
+    }
+
+    if (!this.#tap) {
+      throw new Error('cannot mock packages without a tap fixture')
+    }
+
+    const reqheaders = {}
+    if (this.#authorization) {
+      reqheaders.authorization = `Bearer ${this.#authorization}`
+    }
+    if (this.#basic) {
+      reqheaders.authorization = `Basic ${this.#basic}`
+    }
+
+    this.nock = MockRegistry.tnock(
+      this.#tap,
+      this.#registry,
+      { reqheaders },
+      { debug: this.#debug, strict: this.#strict }
+    )
+  }
+
   search ({ responseCode = 200, results = [], error }) {
     // the flags, score, and searchScore parts of the response are never used
     // by npm, only package is used
@@ -296,13 +332,14 @@ class MockRegistry {
       manifest.users = users
     }
     for (const packument of packuments) {
+      const unscoped = name.includes('/') ? name.split('/')[1] : name
       manifest.versions[packument.version] = {
         _id: `${name}@${packument.version}`,
         name,
         description: 'test package mock manifest',
         dependencies: {},
         dist: {
-          tarball: `${this.#registry}/${name}/-/${name}-${packument.version}.tgz`,
+          tarball: `${this.#registry}/${name}/-/${unscoped}-${packument.version}.tgz`,
         },
         maintainers: [],
         ...packument,

package-lock.json

@@ -15105,6 +15105,7 @@
         "@npmcli/arborist": "^6.1.2",
         "@npmcli/ci-detect": "^3.0.1",
         "@npmcli/run-script": "^6.0.0",
+        "bin-links": "^4.0.1",
         "chalk": "^4.1.0",
         "npm-package-arg": "^10.0.0",
         "npmlog": "^7.0.1",
@@ -15117,8 +15118,10 @@
       },
       "devDependencies": {
         "@npmcli/eslint-config": "^4.0.0",
+        "@npmcli/mock-registry": "^1.0.0",
         "@npmcli/template-oss": "4.10.0",
-        "bin-links": "^4.0.1",
+        "just-extend": "^6.1.1",
+        "just-safe-set": "^4.1.1",
         "minify-registry-metadata": "^2.2.0",
         "mkdirp": "^1.0.4",
         "tap": "^16.0.1"

smoke-tests/test/fixtures/setup.js

@@ -84,6 +84,7 @@ module.exports = async (t, { testdir = {}, debug } = {}) => {
     tap: t,
     registry: 'http://smoke-test-registry.club/',
     debug,
+    strict: true,
   })
   const httpProxyRegistry = `http://localhost:${PORT}`
   const proxy = httpProxy.createProxyServer({})
@@ -92,12 +93,8 @@ module.exports = async (t, { testdir = {}, debug } = {}) => {
   t.teardown(() => server.close())
 
   // update notifier should never be written
-  t.afterEach(async (t) => {
+  t.afterEach((t) => {
     t.equal(existsSync(join(paths.cache, '_update-notifier-last-checked')), false)
-    // this requires that mocks not be shared between sub tests but it helps
-    // find mistakes quicker instead of waiting for the entire test to end
-    t.strictSame(registry.nock.pendingMocks(), [], 'no pending mocks after each')
-    t.strictSame(registry.nock.activeMocks(), [], 'no active mocks after each')
   })
 
   const debugLog = debug || CI ? (...a) => console.error(...a) : () => {}

workspaces/libnpmexec/lib/index.js

@@ -1,6 +1,6 @@
 'use strict'
 
-const { mkdir } = require('fs/promises')
+const { mkdir, symlink, unlink, rm } = require('fs/promises')
 const { promisify } = require('util')
 
 const Arborist = require('@npmcli/arborist')
@@ -12,6 +12,8 @@ const npmlog = require('npmlog')
 const pacote = require('pacote')
 const read = promisify(require('read'))
 const semver = require('semver')
+const binLinks = require('bin-links')
+binLinks.linkBins = require('bin-links/lib/link-bins')
 
 const { fileExists, localFileExists } = require('./file-exists.js')
 const getBinFromManifest = require('./get-bin-from-manifest.js')
@@ -117,31 +119,64 @@ const exec = async (opts) => {
   // - in the local tree
   // - globally
   if (needPackageCommandSwap) {
-    let localManifest
-    try {
-      localManifest = await pacote.manifest(path, flatOptions)
-    } catch {
-      // no local package.json? no problem, move one.
-    }
-    if (localManifest?.bin?.[args[0]]) {
-      // we have to install the local package into the npx cache so that its
-      // bin links get set up
-      packages.push(path)
-      yes = true
-      flatOptions.installLinks = false
-    } else {
-      const dir = dirname(dirname(localBin))
-      const localBinPath = await localFileExists(dir, args[0], '/')
-      if (localBinPath) {
-        binPaths.push(localBinPath)
-        return await run()
-      } else if (globalPath && await fileExists(`${globalBin}/${args[0]}`)) {
-        binPaths.push(globalBin)
-        return await run()
+    const localManifest = await pacote.manifest(path, flatOptions).catch(() => null)
+    const manifestBinPath = localManifest?.bin?.[args[0]]
+    if (manifestBinPath && await fileExists(resolve(path, manifestBinPath))) {
+      const tmpNmPath = resolve(path, 'node_modules', localManifest.name)
+      const tmpDir = await mkdir(dirname(tmpNmPath), { recursive: true })
+      await symlink(path, tmpNmPath, 'dir')
+
+      // only link the bin that we already know exists in case anything else would fail
+      const binOpts = {
+        force: false,
+        path: tmpNmPath,
+        pkg: { bin: { [args[0]]: manifestBinPath } },
+      }
+      // binLinks returns null if it was created and false if not so if we have
+      // a valid path here then we can keep going. if we did not create a bin
+      // here then keep trying the next steps below, since there is probably
+      // a bin that is already linked there which we will run.
+      const linkedBins = await binLinks.linkBins(binOpts).catch(() => []).then((r) => {
+        return r[0] === null ? binLinks.getPaths(binOpts) : []
+      })
+
+      const cleanupLinks = async () => {
+        // always unlink symlinks when we are done
+        await unlink(tmpNmPath)
+        if (linkedBins.length) {
+          await Promise.all(linkedBins.map(b => unlink(b)))
+        }
+        // Only if mkdir indicated that it created a dir should we cleanup
+        // that directory
+        if (tmpDir) {
+          await rm(tmpDir, { recursive: true, force: true })
+        }
+      }
+
+      if (linkedBins.length) {
+        binPaths.push(path)
+        return await run().finally(cleanupLinks)
+      } else {
+        // if we didnt create a bin then we still might need to cleanup the
+        // symlinked directory we created
+        await cleanupLinks()
       }
-      // We swap out args[0] with the bin from the manifest later
-      packages.push(args[0])
     }
+
+    const localBinDir = dirname(dirname(localBin))
+    const localBinPath = await localFileExists(localBinDir, args[0], '/')
+    if (localBinPath) {
+      binPaths.push(localBinPath)
+      return await run()
+    }
+
+    if (globalPath && await fileExists(`${globalBin}/${args[0]}`)) {
+      binPaths.push(globalBin)
+      return await run()
+    }
+
+    // We swap out args[0] with the bin from the manifest later
+    packages.push(args[0])
   }
 
   // Resolve any directory specs so that the npx directory is unique to the

workspaces/libnpmexec/package.json

@@ -51,8 +51,10 @@
   },
   "devDependencies": {
     "@npmcli/eslint-config": "^4.0.0",
+    "@npmcli/mock-registry": "^1.0.0",
     "@npmcli/template-oss": "4.10.0",
-    "bin-links": "^4.0.1",
+    "just-extend": "^6.1.1",
+    "just-safe-set": "^4.1.1",
     "minify-registry-metadata": "^2.2.0",
     "mkdirp": "^1.0.4",
     "tap": "^16.0.1"
@@ -61,6 +63,7 @@
     "@npmcli/arborist": "^6.1.2",
     "@npmcli/ci-detect": "^3.0.1",
     "@npmcli/run-script": "^6.0.0",
+    "bin-links": "^4.0.1",
     "chalk": "^4.1.0",
     "npm-package-arg": "^10.0.0",
     "npmlog": "^7.0.1",