Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 3 commits into
base: latest
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions DEPENDENCIES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
81 changes: 59 additions & 22 deletions mock-registry/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class MockRegistry {
#authorization
#basic
#debug
#strict

constructor (opts) {
if (!opts.registry) {
Expand All @@ -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
}

Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion package-lock.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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"
Expand Down
7 changes: 2 additions & 5 deletions smoke-tests/test/fixtures/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({})
Expand All @@ -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) : () => {}
Expand Down
83 changes: 59 additions & 24 deletions workspaces/libnpmexec/lib/index.js
Original file line number Diff line number Diff line change
@@ -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')
Expand All @@ -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')
Expand Down Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion workspaces/libnpmexec/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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",
Expand Down
Loading