From e917cd4b526902004a0232c8474445459ef0c7e4 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 14:18:18 -0700 Subject: [PATCH 01/16] fix(cleanup): dont nest files utils dir --- lib/base-command.js | 2 +- lib/commands/edit.js | 2 +- lib/commands/explain.js | 2 +- lib/commands/explore.js | 2 +- lib/commands/fund.js | 2 +- lib/commands/init.js | 2 +- lib/commands/ls.js | 2 +- lib/commands/rebuild.js | 2 +- lib/commands/uninstall.js | 2 +- lib/commands/update.js | 2 +- lib/commands/version.js | 2 +- lib/{workspaces => utils}/get-workspaces.js | 0 lib/utils/{completion => }/installed-deep.js | 0 lib/utils/{completion => }/installed-shallow.js | 0 lib/{workspaces => utils}/update-workspaces.js | 0 test/lib/arborist-cmd.js | 2 +- test/lib/{workspaces => utils}/get-workspaces.js | 2 +- test/lib/utils/{completion => }/installed-deep.js | 2 +- test/lib/utils/{completion => }/installed-shallow.js | 2 +- 19 files changed, 15 insertions(+), 15 deletions(-) rename lib/{workspaces => utils}/get-workspaces.js (100%) rename lib/utils/{completion => }/installed-deep.js (100%) rename lib/utils/{completion => }/installed-shallow.js (100%) rename lib/{workspaces => utils}/update-workspaces.js (100%) rename test/lib/{workspaces => utils}/get-workspaces.js (98%) rename test/lib/utils/{completion => }/installed-deep.js (97%) rename test/lib/utils/{completion => }/installed-shallow.js (93%) diff --git a/lib/base-command.js b/lib/base-command.js index cfaa0bb394460..fb33005cc901b 100644 --- a/lib/base-command.js +++ b/lib/base-command.js @@ -167,7 +167,7 @@ class BaseCommand { const relativeFrom = prefixInsideCwd ? this.npm.localPrefix : process.cwd() const filters = this.npm.config.get('workspace') - const getWorkspaces = require('./workspaces/get-workspaces.js') + const getWorkspaces = require('./utils/get-workspaces.js') const ws = await getWorkspaces(filters, { path: this.npm.localPrefix, includeWorkspaceRoot, diff --git a/lib/commands/edit.js b/lib/commands/edit.js index e6ab26278740c..eca4fad6fe24c 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -4,7 +4,7 @@ const { resolve } = require('path') const { lstat } = require('fs/promises') const cp = require('child_process') -const completion = require('../utils/completion/installed-shallow.js') +const completion = require('../utils/installed-shallow.js') const BaseCommand = require('../base-command.js') const splitPackageNames = (path) => { diff --git a/lib/commands/explain.js b/lib/commands/explain.js index 98ef356bfc1b3..1e09ea0ee61cb 100644 --- a/lib/commands/explain.js +++ b/lib/commands/explain.js @@ -20,7 +20,7 @@ class Explain extends ArboristWorkspaceCmd { // TODO /* istanbul ignore next */ static async completion (opts, npm) { - const completion = require('../utils/completion/installed-deep.js') + const completion = require('../utils/installed-deep.js') return completion(npm, opts) } diff --git a/lib/commands/explore.js b/lib/commands/explore.js index ef4743e62197d..ae29262071555 100644 --- a/lib/commands/explore.js +++ b/lib/commands/explore.js @@ -5,7 +5,7 @@ const pkgJson = require('@npmcli/package-json') const runScript = require('@npmcli/run-script') const { join, relative } = require('path') const { log, output } = require('proc-log') -const completion = require('../utils/completion/installed-shallow.js') +const completion = require('../utils/installed-shallow.js') const BaseCommand = require('../base-command.js') class Explore extends BaseCommand { diff --git a/lib/commands/fund.js b/lib/commands/fund.js index 6fa2c467a35eb..5aaab2df746a5 100644 --- a/lib/commands/fund.js +++ b/lib/commands/fund.js @@ -38,7 +38,7 @@ class Fund extends ArboristWorkspaceCmd { // TODO /* istanbul ignore next */ static async completion (opts, npm) { - const completion = require('../utils/completion/installed-deep.js') + const completion = require('../utils/installed-deep.js') return completion(npm, opts) } diff --git a/lib/commands/init.js b/lib/commands/init.js index 28067e4def1d0..cee56e7b91ba5 100644 --- a/lib/commands/init.js +++ b/lib/commands/init.js @@ -7,7 +7,7 @@ const libexec = require('libnpmexec') const mapWorkspaces = require('@npmcli/map-workspaces') const PackageJson = require('@npmcli/package-json') const { log, output } = require('proc-log') -const updateWorkspaces = require('../workspaces/update-workspaces.js') +const updateWorkspaces = require('../utils/update-workspaces.js') const BaseCommand = require('../base-command.js') const posixPath = p => p.split('\\').join('/') diff --git a/lib/commands/ls.js b/lib/commands/ls.js index a81e69fe40e89..cf086924092fe 100644 --- a/lib/commands/ls.js +++ b/lib/commands/ls.js @@ -42,7 +42,7 @@ class LS extends ArboristWorkspaceCmd { // TODO /* istanbul ignore next */ static async completion (opts, npm) { - const completion = require('../utils/completion/installed-deep.js') + const completion = require('../utils/installed-deep.js') return completion(npm, opts) } diff --git a/lib/commands/rebuild.js b/lib/commands/rebuild.js index 8858edd1da349..d9e22de7cf048 100644 --- a/lib/commands/rebuild.js +++ b/lib/commands/rebuild.js @@ -20,7 +20,7 @@ class Rebuild extends ArboristWorkspaceCmd { // TODO /* istanbul ignore next */ static async completion (opts, npm) { - const completion = require('../utils/completion/installed-deep.js') + const completion = require('../utils/installed-deep.js') return completion(npm, opts) } diff --git a/lib/commands/uninstall.js b/lib/commands/uninstall.js index 07775efb9cf2f..51df355d5a899 100644 --- a/lib/commands/uninstall.js +++ b/lib/commands/uninstall.js @@ -2,7 +2,7 @@ const { resolve } = require('path') const pkgJson = require('@npmcli/package-json') const reifyFinish = require('../utils/reify-finish.js') -const completion = require('../utils/completion/installed-shallow.js') +const completion = require('../utils/installed-shallow.js') const ArboristWorkspaceCmd = require('../arborist-cmd.js') class Uninstall extends ArboristWorkspaceCmd { diff --git a/lib/commands/update.js b/lib/commands/update.js index 12b6ac153a588..63711c05e5b97 100644 --- a/lib/commands/update.js +++ b/lib/commands/update.js @@ -33,7 +33,7 @@ class Update extends ArboristWorkspaceCmd { // TODO /* istanbul ignore next */ static async completion (opts, npm) { - const completion = require('../utils/completion/installed-deep.js') + const completion = require('../utils/installed-deep.js') return completion(npm, opts) } diff --git a/lib/commands/version.js b/lib/commands/version.js index 376cd88480d65..0e1916d7af7fb 100644 --- a/lib/commands/version.js +++ b/lib/commands/version.js @@ -81,7 +81,7 @@ class Version extends BaseCommand { } async changeWorkspaces (args) { - const updateWorkspaces = require('../workspaces/update-workspaces.js') + const updateWorkspaces = require('../utils/update-workspaces.js') const libnpmversion = require('libnpmversion') const prefix = this.npm.config.get('tag-version-prefix') const { diff --git a/lib/workspaces/get-workspaces.js b/lib/utils/get-workspaces.js similarity index 100% rename from lib/workspaces/get-workspaces.js rename to lib/utils/get-workspaces.js diff --git a/lib/utils/completion/installed-deep.js b/lib/utils/installed-deep.js similarity index 100% rename from lib/utils/completion/installed-deep.js rename to lib/utils/installed-deep.js diff --git a/lib/utils/completion/installed-shallow.js b/lib/utils/installed-shallow.js similarity index 100% rename from lib/utils/completion/installed-shallow.js rename to lib/utils/installed-shallow.js diff --git a/lib/workspaces/update-workspaces.js b/lib/utils/update-workspaces.js similarity index 100% rename from lib/workspaces/update-workspaces.js rename to lib/utils/update-workspaces.js diff --git a/test/lib/arborist-cmd.js b/test/lib/arborist-cmd.js index 79ff8c6c24601..3c3f44f75d89f 100644 --- a/test/lib/arborist-cmd.js +++ b/test/lib/arborist-cmd.js @@ -127,7 +127,7 @@ t.test('arborist-cmd', async t => { t.test('handle getWorkspaces raising an error', async t => { const { cmd } = await mockArboristCmd(t, null, 'a', { mocks: { - '{LIB}/workspaces/get-workspaces.js': async () => { + '{LIB}/utils/get-workspaces.js': async () => { throw new Error('oopsie') }, }, diff --git a/test/lib/workspaces/get-workspaces.js b/test/lib/utils/get-workspaces.js similarity index 98% rename from test/lib/workspaces/get-workspaces.js rename to test/lib/utils/get-workspaces.js index 4e48b1c4b7707..931f90d13046f 100644 --- a/test/lib/workspaces/get-workspaces.js +++ b/test/lib/utils/get-workspaces.js @@ -1,6 +1,6 @@ const { resolve } = require('path') const t = require('tap') -const getWorkspaces = require('../../../lib/workspaces/get-workspaces.js') +const getWorkspaces = require('../../../lib/utils/get-workspaces.js') const normalizePath = p => p .replace(/\\+/g, '/') diff --git a/test/lib/utils/completion/installed-deep.js b/test/lib/utils/installed-deep.js similarity index 97% rename from test/lib/utils/completion/installed-deep.js rename to test/lib/utils/installed-deep.js index 0af26861ff83a..4d212746cba89 100644 --- a/test/lib/utils/completion/installed-deep.js +++ b/test/lib/utils/installed-deep.js @@ -1,5 +1,5 @@ const t = require('tap') -const installedDeep = require('../../../../lib/utils/completion/installed-deep.js') +const installedDeep = require('../../../lib/utils/installed-deep.js') const mockNpm = require('../../../fixtures/mock-npm') const fixture = { diff --git a/test/lib/utils/completion/installed-shallow.js b/test/lib/utils/installed-shallow.js similarity index 93% rename from test/lib/utils/completion/installed-shallow.js rename to test/lib/utils/installed-shallow.js index 3666803979cb3..43ccda4daea69 100644 --- a/test/lib/utils/completion/installed-shallow.js +++ b/test/lib/utils/installed-shallow.js @@ -1,5 +1,5 @@ const t = require('tap') -const installed = require('../../../../lib/utils/completion/installed-shallow.js') +const installed = require('../../../lib/utils/installed-shallow.js') const mockNpm = require('../../../fixtures/mock-npm') const mockShallow = async (t, config) => { From 0bfabc29d1ec585e0c332efe0ff7d40dd9e6d423 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 14:28:58 -0700 Subject: [PATCH 02/16] fix(cleanup): newlines and whitespace --- lib/base-command.js | 1 + lib/commands/access.js | 1 - lib/commands/adduser.js | 2 +- lib/commands/audit.js | 388 +------------------------------ lib/commands/ci.js | 1 - lib/commands/completion.js | 11 +- lib/commands/config.js | 2 +- lib/commands/dedupe.js | 3 +- lib/commands/dist-tag.js | 1 + lib/commands/docs.js | 2 + lib/commands/doctor.js | 3 +- lib/commands/edit.js | 42 ++-- lib/commands/explain.js | 1 + lib/commands/explore.js | 6 +- lib/commands/find-dupes.js | 3 +- lib/commands/fund.js | 1 + lib/commands/get.js | 1 + lib/commands/help-search.js | 1 + lib/commands/help.js | 3 +- lib/commands/hook.js | 3 +- lib/commands/install-ci-test.js | 6 +- lib/commands/install-test.js | 6 +- lib/commands/install.js | 3 +- lib/commands/link.js | 1 + lib/commands/login.js | 2 +- lib/commands/logout.js | 1 + lib/commands/ls.js | 9 +- lib/commands/org.js | 1 + lib/commands/outdated.js | 2 +- lib/commands/pack.js | 1 + lib/commands/ping.js | 1 + lib/commands/prefix.js | 1 + lib/commands/profile.js | 6 +- lib/commands/prune.js | 5 +- lib/commands/publish.js | 5 +- lib/commands/query.js | 2 - lib/commands/rebuild.js | 3 +- lib/commands/repo.js | 3 +- lib/commands/root.js | 2 + lib/commands/run-script.js | 2 +- lib/commands/sbom.js | 2 - lib/commands/search.js | 2 +- lib/commands/set.js | 1 + lib/commands/shrinkwrap.js | 1 + lib/commands/star.js | 3 +- lib/commands/stars.js | 3 +- lib/commands/team.js | 2 +- lib/commands/token.js | 4 +- lib/commands/uninstall.js | 4 +- lib/commands/unpublish.js | 3 +- lib/commands/unstar.js | 1 + lib/commands/update.js | 5 +- lib/commands/version.js | 1 - lib/commands/view.js | 5 +- lib/commands/whoami.js | 3 +- lib/lifecycle-cmd.js | 1 + lib/npm.js | 1 + lib/package-url-cmd.js | 1 + lib/utils/did-you-mean.js | 1 + lib/utils/verify-signatures.js | 389 ++++++++++++++++++++++++++++++++ 60 files changed, 499 insertions(+), 472 deletions(-) create mode 100644 lib/utils/verify-signatures.js diff --git a/lib/base-command.js b/lib/base-command.js index fb33005cc901b..9c30ef8a4b44e 100644 --- a/lib/base-command.js +++ b/lib/base-command.js @@ -179,4 +179,5 @@ class BaseCommand { this.workspacePaths = [...ws.values()] } } + module.exports = BaseCommand diff --git a/lib/commands/access.js b/lib/commands/access.js index 20565e274398e..1d0161cadc91e 100644 --- a/lib/commands/access.js +++ b/lib/commands/access.js @@ -3,7 +3,6 @@ const npa = require('npm-package-arg') const { output } = require('proc-log') const pkgJson = require('@npmcli/package-json') const localeCompare = require('@isaacs/string-locale-compare')('en') - const otplease = require('../utils/otplease.js') const getIdentity = require('../utils/get-identity.js') const BaseCommand = require('../base-command.js') diff --git a/lib/commands/adduser.js b/lib/commands/adduser.js index 842819f2bf44b..d896272bf7ed5 100644 --- a/lib/commands/adduser.js +++ b/lib/commands/adduser.js @@ -1,7 +1,6 @@ const { log, output } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const auth = require('../utils/auth.js') - const BaseCommand = require('../base-command.js') class AddUser extends BaseCommand { @@ -47,4 +46,5 @@ class AddUser extends BaseCommand { output.standard(message) } } + module.exports = AddUser diff --git a/lib/commands/audit.js b/lib/commands/audit.js index 7eada9f617e70..aed1be7a82906 100644 --- a/lib/commands/audit.js +++ b/lib/commands/audit.js @@ -1,395 +1,9 @@ const npmAuditReport = require('npm-audit-report') -const fetch = require('npm-registry-fetch') -const localeCompare = require('@isaacs/string-locale-compare')('en') -const npa = require('npm-package-arg') -const pacote = require('pacote') -const pMap = require('p-map') -const tufClient = require('@sigstore/tuf') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') const auditError = require('../utils/audit-error.js') const { log, output } = require('proc-log') const reifyFinish = require('../utils/reify-finish.js') - -const sortAlphabetically = (a, b) => localeCompare(a.name, b.name) - -class VerifySignatures { - constructor (tree, filterSet, npm, opts) { - this.tree = tree - this.filterSet = filterSet - this.npm = npm - this.opts = opts - this.keys = new Map() - this.invalid = [] - this.missing = [] - this.checkedPackages = new Set() - this.auditedWithKeysCount = 0 - this.verifiedSignatureCount = 0 - this.verifiedAttestationCount = 0 - this.exitCode = 0 - } - - async run () { - const start = process.hrtime.bigint() - - // Find all deps in tree - const { edges, registries } = this.getEdgesOut(this.tree.inventory.values(), this.filterSet) - if (edges.size === 0) { - throw new Error('found no installed dependencies to audit') - } - - const tuf = await tufClient.initTUF({ - cachePath: this.opts.tufCache, - retry: this.opts.retry, - timeout: this.opts.timeout, - }) - await Promise.all([...registries].map(registry => this.setKeys({ registry, tuf }))) - - log.verbose('verifying registry signatures') - await pMap(edges, (e) => this.getVerifiedInfo(e), { concurrency: 20, stopOnError: true }) - - // Didn't find any dependencies that could be verified, e.g. only local - // deps, missing version, not on a registry etc. - if (!this.auditedWithKeysCount) { - throw new Error('found no dependencies to audit that were installed from ' + - 'a supported registry') - } - - const invalid = this.invalid.sort(sortAlphabetically) - const missing = this.missing.sort(sortAlphabetically) - - const hasNoInvalidOrMissing = invalid.length === 0 && missing.length === 0 - - if (!hasNoInvalidOrMissing) { - process.exitCode = 1 - } - - if (this.npm.config.get('json')) { - output.standard(JSON.stringify({ - invalid, - missing, - }, null, 2)) - return - } - const end = process.hrtime.bigint() - const elapsed = end - start - - const auditedPlural = this.auditedWithKeysCount > 1 ? 's' : '' - const timing = `audited ${this.auditedWithKeysCount} package${auditedPlural} in ` + - `${Math.floor(Number(elapsed) / 1e9)}s` - output.standard(timing) - output.standard('') - - const verifiedBold = this.npm.chalk.bold('verified') - if (this.verifiedSignatureCount) { - if (this.verifiedSignatureCount === 1) { - /* eslint-disable-next-line max-len */ - output.standard(`${this.verifiedSignatureCount} package has a ${verifiedBold} registry signature`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`${this.verifiedSignatureCount} packages have ${verifiedBold} registry signatures`) - } - output.standard('') - } - - if (this.verifiedAttestationCount) { - if (this.verifiedAttestationCount === 1) { - /* eslint-disable-next-line max-len */ - output.standard(`${this.verifiedAttestationCount} package has a ${verifiedBold} attestation`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`${this.verifiedAttestationCount} packages have ${verifiedBold} attestations`) - } - output.standard('') - } - - if (missing.length) { - const missingClr = this.npm.chalk.redBright('missing') - if (missing.length === 1) { - /* eslint-disable-next-line max-len */ - output.standard(`1 package has a ${missingClr} registry signature but the registry is providing signing keys:`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`${missing.length} packages have ${missingClr} registry signatures but the registry is providing signing keys:`) - } - output.standard('') - missing.map(m => - output.standard(`${this.npm.chalk.red(`${m.name}@${m.version}`)} (${m.registry})`) - ) - } - - if (invalid.length) { - if (missing.length) { - output.standard('') - } - const invalidClr = this.npm.chalk.redBright('invalid') - // We can have either invalid signatures or invalid provenance - const invalidSignatures = this.invalid.filter(i => i.code === 'EINTEGRITYSIGNATURE') - if (invalidSignatures.length) { - if (invalidSignatures.length === 1) { - output.standard(`1 package has an ${invalidClr} registry signature:`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`${invalidSignatures.length} packages have ${invalidClr} registry signatures:`) - } - output.standard('') - invalidSignatures.map(i => - output.standard(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`) - ) - output.standard('') - } - - const invalidAttestations = this.invalid.filter(i => i.code === 'EATTESTATIONVERIFY') - if (invalidAttestations.length) { - if (invalidAttestations.length === 1) { - output.standard(`1 package has an ${invalidClr} attestation:`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`${invalidAttestations.length} packages have ${invalidClr} attestations:`) - } - output.standard('') - invalidAttestations.map(i => - output.standard(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`) - ) - output.standard('') - } - - if (invalid.length === 1) { - /* eslint-disable-next-line max-len */ - output.standard(`Someone might have tampered with this package since it was published on the registry!`) - } else { - /* eslint-disable-next-line max-len */ - output.standard(`Someone might have tampered with these packages since they were published on the registry!`) - } - output.standard('') - } - } - - getEdgesOut (nodes, filterSet) { - const edges = new Set() - const registries = new Set() - for (const node of nodes) { - for (const edge of node.edgesOut.values()) { - const filteredOut = - edge.from - && filterSet - && filterSet.size > 0 - && !filterSet.has(edge.from.target) - - if (!filteredOut) { - const spec = this.getEdgeSpec(edge) - if (spec) { - // Prefetch and cache public keys from used registries - registries.add(this.getSpecRegistry(spec)) - } - edges.add(edge) - } - } - } - return { edges, registries } - } - - async setKeys ({ registry, tuf }) { - const { host, pathname } = new URL(registry) - // Strip any trailing slashes from pathname - const regKey = `${host}${pathname.replace(/\/$/, '')}/keys.json` - let keys = await tuf.getTarget(regKey) - .then((target) => JSON.parse(target)) - .then(({ keys: ks }) => ks.map((key) => ({ - ...key, - keyid: key.keyId, - pemkey: `-----BEGIN PUBLIC KEY-----\n${key.publicKey.rawBytes}\n-----END PUBLIC KEY-----`, - expires: key.publicKey.validFor.end || null, - }))).catch(err => { - if (err.code === 'TUF_FIND_TARGET_ERROR') { - return null - } else { - throw err - } - }) - - // If keys not found in Sigstore TUF repo, fallback to registry keys API - if (!keys) { - keys = await fetch.json('/-/npm/v1/keys', { - ...this.npm.flatOptions, - registry, - }).then(({ keys: ks }) => ks.map((key) => ({ - ...key, - pemkey: `-----BEGIN PUBLIC KEY-----\n${key.key}\n-----END PUBLIC KEY-----`, - }))).catch(err => { - if (err.code === 'E404' || err.code === 'E400') { - return null - } else { - throw err - } - }) - } - - if (keys) { - this.keys.set(registry, keys) - } - } - - getEdgeType (edge) { - return edge.optional ? 'optionalDependencies' - : edge.peer ? 'peerDependencies' - : edge.dev ? 'devDependencies' - : 'dependencies' - } - - getEdgeSpec (edge) { - let name = edge.name - try { - name = npa(edge.spec).subSpec.name - } catch { - // leave it as edge.name - } - try { - return npa(`${name}@${edge.spec}`) - } catch { - // Skip packages with invalid spec - } - } - - buildRegistryConfig (registry) { - const keys = this.keys.get(registry) || [] - const parsedRegistry = new URL(registry) - const regKey = `//${parsedRegistry.host}${parsedRegistry.pathname}` - return { - [`${regKey}:_keys`]: keys, - } - } - - getSpecRegistry (spec) { - return fetch.pickRegistry(spec, this.npm.flatOptions) - } - - getValidPackageInfo (edge) { - const type = this.getEdgeType(edge) - // Skip potentially optional packages that are not on disk, as these could - // be omitted during install - if (edge.error === 'MISSING' && type !== 'dependencies') { - return - } - - const spec = this.getEdgeSpec(edge) - // Skip invalid version requirements - if (!spec) { - return - } - const node = edge.to || edge - const { version } = node.package || {} - - if (node.isWorkspace || // Skip local workspaces packages - !version || // Skip packages that don't have a installed version, e.g. optonal dependencies - !spec.registry) { // Skip if not from registry, e.g. git package - return - } - - for (const omitType of this.npm.config.get('omit')) { - if (node[omitType]) { - return - } - } - - return { - name: spec.name, - version, - type, - location: node.location, - registry: this.getSpecRegistry(spec), - } - } - - async verifySignatures (name, version, registry) { - const { - _integrity: integrity, - _signatures, - _attestations, - _resolved: resolved, - } = await pacote.manifest(`${name}@${version}`, { - verifySignatures: true, - verifyAttestations: true, - ...this.buildRegistryConfig(registry), - ...this.npm.flatOptions, - }) - const signatures = _signatures || [] - const result = { - integrity, - signatures, - attestations: _attestations, - resolved, - } - return result - } - - async getVerifiedInfo (edge) { - const info = this.getValidPackageInfo(edge) - if (!info) { - return - } - const { name, version, location, registry, type } = info - if (this.checkedPackages.has(location)) { - // we already did or are doing this one - return - } - this.checkedPackages.add(location) - - // We only "audit" or verify the signature, or the presence of it, on - // packages whose registry returns signing keys - const keys = this.keys.get(registry) || [] - if (keys.length) { - this.auditedWithKeysCount += 1 - } - - try { - const { integrity, signatures, attestations, resolved } = await this.verifySignatures( - name, version, registry - ) - - // Currently we only care about missing signatures on registries that provide a public key - // We could make this configurable in the future with a strict/paranoid mode - if (signatures.length) { - this.verifiedSignatureCount += 1 - } else if (keys.length) { - this.missing.push({ - integrity, - location, - name, - registry, - resolved, - version, - }) - } - - // Track verified attestations separately to registry signatures, as all - // packages on registries with signing keys are expected to have registry - // signatures, but not all packages have provenance and publish attestations. - if (attestations) { - this.verifiedAttestationCount += 1 - } - } catch (e) { - if (e.code === 'EINTEGRITYSIGNATURE' || e.code === 'EATTESTATIONVERIFY') { - this.invalid.push({ - code: e.code, - message: e.message, - integrity: e.integrity, - keyid: e.keyid, - location, - name, - registry, - resolved: e.resolved, - signature: e.signature, - predicateType: e.predicateType, - type, - version, - }) - } else { - throw e - } - } - } -} +const VerifySignatures = require('../utils/verify-signatures.js') class Audit extends ArboristWorkspaceCmd { static description = 'Run a security audit' diff --git a/lib/commands/ci.js b/lib/commands/ci.js index ff559b6d801d7..a8b4a845126f5 100644 --- a/lib/commands/ci.js +++ b/lib/commands/ci.js @@ -3,7 +3,6 @@ const runScript = require('@npmcli/run-script') const fs = require('fs/promises') const { log, time } = require('proc-log') const validateLockfile = require('../utils/validate-lockfile.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') class CI extends ArboristWorkspaceCmd { diff --git a/lib/commands/completion.js b/lib/commands/completion.js index 677d7fa2ec3fe..20ff3ea76e98b 100644 --- a/lib/commands/completion.js +++ b/lib/commands/completion.js @@ -27,23 +27,22 @@ // Matches are wrapped with ' to escape them, if necessary, and then printed // one per line for the shell completion method to consume in IFS=$'\n' mode // as an array. -// const fs = require('fs/promises') const nopt = require('nopt') const { resolve } = require('path') const { output } = require('proc-log') - const Npm = require('../npm.js') const { definitions, shorthands } = require('@npmcli/config/lib/definitions') const { commands, aliases, deref } = require('../utils/cmd-list.js') -const configNames = Object.keys(definitions) -const shorthandNames = Object.keys(shorthands) -const allConfs = configNames.concat(shorthandNames) const { isWindowsShell } = require('../utils/is-windows.js') +const BaseCommand = require('../base-command.js') + const fileExists = (file) => fs.stat(file).then(s => s.isFile()).catch(() => false) -const BaseCommand = require('../base-command.js') +const configNames = Object.keys(definitions) +const shorthandNames = Object.keys(shorthands) +const allConfs = configNames.concat(shorthandNames) class Completion extends BaseCommand { static description = 'Tab Completion for npm' diff --git a/lib/commands/config.js b/lib/commands/config.js index 21faa8838c593..56315208e232f 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -6,6 +6,7 @@ const localeCompare = require('@isaacs/string-locale-compare')('en') const pkgJson = require('@npmcli/package-json') const { defaults, definitions } = require('@npmcli/config/lib/definitions') const { log, output } = require('proc-log') +const BaseCommand = require('../base-command.js') // These are the configs that we can nerf-dart. Not all of them currently even // *have* config definitions so we have to explicitly validate them here @@ -46,7 +47,6 @@ const publicVar = k => { return true } -const BaseCommand = require('../base-command.js') class Config extends BaseCommand { static description = 'Manage the npm configuration files' static name = 'config' diff --git a/lib/commands/dedupe.js b/lib/commands/dedupe.js index 0d0e26621b227..f9785fd66bb49 100644 --- a/lib/commands/dedupe.js +++ b/lib/commands/dedupe.js @@ -1,8 +1,7 @@ -// dedupe duplicated packages, or find them in the tree const reifyFinish = require('../utils/reify-finish.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') +// dedupe duplicated packages, or find them in the tree class Dedupe extends ArboristWorkspaceCmd { static description = 'Reduce duplication in the package tree' static name = 'dedupe' diff --git a/lib/commands/dist-tag.js b/lib/commands/dist-tag.js index 741890fc9cec7..7eeeae571bc1c 100644 --- a/lib/commands/dist-tag.js +++ b/lib/commands/dist-tag.js @@ -205,4 +205,5 @@ class DistTag extends BaseCommand { return data } } + module.exports = DistTag diff --git a/lib/commands/docs.js b/lib/commands/docs.js index 5d20215b56a07..2259b49f79617 100644 --- a/lib/commands/docs.js +++ b/lib/commands/docs.js @@ -1,4 +1,5 @@ const PackageUrlCmd = require('../package-url-cmd.js') + class Docs extends PackageUrlCmd { static description = 'Open documentation for a package in a web browser' static name = 'docs' @@ -16,4 +17,5 @@ class Docs extends PackageUrlCmd { return `https://www.npmjs.com/package/${mani.name}` } } + module.exports = Docs diff --git a/lib/commands/doctor.js b/lib/commands/doctor.js index 2c8581a4aba4d..a9f24bd62e0f0 100644 --- a/lib/commands/doctor.js +++ b/lib/commands/doctor.js @@ -8,6 +8,7 @@ const semver = require('semver') const { log, output } = require('proc-log') const ping = require('../utils/ping.js') const { defaults } = require('@npmcli/config/lib/definitions') +const BaseCommand = require('../base-command.js') const maskLabel = mask => { const label = [] @@ -93,7 +94,7 @@ const subcommands = [ // - verify all local packages have bins linked // What is the fix for these? ] -const BaseCommand = require('../base-command.js') + class Doctor extends BaseCommand { static description = 'Check the health of your npm environment' static name = 'doctor' diff --git a/lib/commands/edit.js b/lib/commands/edit.js index eca4fad6fe24c..12c9f759589e7 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -1,34 +1,31 @@ -// npm edit -// open the package folder in the $EDITOR - const { resolve } = require('path') const { lstat } = require('fs/promises') const cp = require('child_process') const completion = require('../utils/installed-shallow.js') const BaseCommand = require('../base-command.js') -const splitPackageNames = (path) => { - return path.split('/') - // combine scoped parts - .reduce((parts, part) => { - if (parts.length === 0) { - return [part] - } +const splitPackageNames = (path) => path.split('/') +// combine scoped parts + .reduce((parts, part) => { + if (parts.length === 0) { + return [part] + } - const lastPart = parts[parts.length - 1] - // check if previous part is the first part of a scoped package - if (lastPart[0] === '@' && !lastPart.includes('/')) { - parts[parts.length - 1] += '/' + part - } else { - parts.push(part) - } + const lastPart = parts[parts.length - 1] + // check if previous part is the first part of a scoped package + if (lastPart[0] === '@' && !lastPart.includes('/')) { + parts[parts.length - 1] += '/' + part + } else { + parts.push(part) + } - return parts - }, []) - .join('/node_modules/') - .replace(/(\/node_modules)+/, '/node_modules') -} + return parts + }, []) + .join('/node_modules/') + .replace(/(\/node_modules)+/, '/node_modules') +// npm edit +// open the package folder in the $EDITOR class Edit extends BaseCommand { static description = 'Edit an installed package' static name = 'edit' @@ -63,4 +60,5 @@ class Edit extends BaseCommand { }) } } + module.exports = Edit diff --git a/lib/commands/explain.js b/lib/commands/explain.js index 1e09ea0ee61cb..2e7d07df729a8 100644 --- a/lib/commands/explain.js +++ b/lib/commands/explain.js @@ -126,4 +126,5 @@ class Explain extends ArboristWorkspaceCmd { }) } } + module.exports = Explain diff --git a/lib/commands/explore.js b/lib/commands/explore.js index ae29262071555..a4acc821eabf9 100644 --- a/lib/commands/explore.js +++ b/lib/commands/explore.js @@ -1,6 +1,3 @@ -// npm explore [@] -// open a subshell to the package folder. - const pkgJson = require('@npmcli/package-json') const runScript = require('@npmcli/run-script') const { join, relative } = require('path') @@ -8,6 +5,8 @@ const { log, output } = require('proc-log') const completion = require('../utils/installed-shallow.js') const BaseCommand = require('../base-command.js') +// npm explore [@] +// open a subshell to the package folder. class Explore extends BaseCommand { static description = 'Browse an installed package' static name = 'explore' @@ -71,4 +70,5 @@ class Explore extends BaseCommand { }) } } + module.exports = Explore diff --git a/lib/commands/find-dupes.js b/lib/commands/find-dupes.js index 2e06e8b6bd93f..0945089c74b7c 100644 --- a/lib/commands/find-dupes.js +++ b/lib/commands/find-dupes.js @@ -1,6 +1,6 @@ -// dedupe duplicated packages, or find them in the tree const ArboristWorkspaceCmd = require('../arborist-cmd.js') +// dedupe duplicated packages, or find them in the tree class FindDupes extends ArboristWorkspaceCmd { static description = 'Find duplication in the package tree' static name = 'find-dupes' @@ -24,4 +24,5 @@ class FindDupes extends ArboristWorkspaceCmd { return this.npm.exec('dedupe', []) } } + module.exports = FindDupes diff --git a/lib/commands/fund.js b/lib/commands/fund.js index 5aaab2df746a5..8bcd184e70968 100644 --- a/lib/commands/fund.js +++ b/lib/commands/fund.js @@ -222,4 +222,5 @@ class Fund extends ArboristWorkspaceCmd { return [url, message] } } + module.exports = Fund diff --git a/lib/commands/get.js b/lib/commands/get.js index 4bf5d2caf8264..43eca7389ec90 100644 --- a/lib/commands/get.js +++ b/lib/commands/get.js @@ -19,4 +19,5 @@ class Get extends BaseCommand { return this.npm.exec('config', ['get'].concat(args)) } } + module.exports = Get diff --git a/lib/commands/help-search.js b/lib/commands/help-search.js index 8821d932525dc..1bbff2df6cc16 100644 --- a/lib/commands/help-search.js +++ b/lib/commands/help-search.js @@ -191,4 +191,5 @@ class HelpSearch extends BaseCommand { return finalOut.trim() } } + module.exports = HelpSearch diff --git a/lib/commands/help.js b/lib/commands/help.js index 1f51713a8d051..fd84d3f8546ef 100644 --- a/lib/commands/help.js +++ b/lib/commands/help.js @@ -5,9 +5,9 @@ const { glob } = require('glob') const { output } = require('proc-log') const localeCompare = require('@isaacs/string-locale-compare')('en') const { deref } = require('../utils/cmd-list.js') +const BaseCommand = require('../base-command.js') const globify = pattern => pattern.split('\\').join('/') -const BaseCommand = require('../base-command.js') // Strips out the number from foo.7 or foo.7. or foo.7.tgz // We don't currently compress our man pages but if we ever did this would @@ -111,4 +111,5 @@ class Help extends BaseCommand { return 'file:///' + path.resolve(this.npm.npmRoot, `docs/output/${sect}/${f}.html`) } } + module.exports = Help diff --git a/lib/commands/hook.js b/lib/commands/hook.js index 7ec95e079d660..557d04e7a035a 100644 --- a/lib/commands/hook.js +++ b/lib/commands/hook.js @@ -2,8 +2,8 @@ const hookApi = require('libnpmhook') const otplease = require('../utils/otplease.js') const relativeDate = require('tiny-relative-date') const { output } = require('proc-log') - const BaseCommand = require('../base-command.js') + class Hook extends BaseCommand { static description = 'Manage registry hooks' static name = 'hook' @@ -105,4 +105,5 @@ class Hook extends BaseCommand { return `${hook.type === 'owner' ? '~' : ''}${hook.name}` } } + module.exports = Hook diff --git a/lib/commands/install-ci-test.js b/lib/commands/install-ci-test.js index f7a357ba6e124..4b9dd269f8c74 100644 --- a/lib/commands/install-ci-test.js +++ b/lib/commands/install-ci-test.js @@ -1,8 +1,7 @@ -// npm install-ci-test -// Runs `npm ci` and then runs `npm test` - const CI = require('./ci.js') +// npm install-ci-test +// Runs `npm ci` and then runs `npm test` class InstallCITest extends CI { static description = 'Install a project with a clean slate and run tests' static name = 'install-ci-test' @@ -12,4 +11,5 @@ class InstallCITest extends CI { return this.npm.exec('test', []) } } + module.exports = InstallCITest diff --git a/lib/commands/install-test.js b/lib/commands/install-test.js index 11f22e535403c..e21ca7c929c55 100644 --- a/lib/commands/install-test.js +++ b/lib/commands/install-test.js @@ -1,8 +1,7 @@ -// npm install-test -// Runs `npm install` and then runs `npm test` - const Install = require('./install.js') +// npm install-test +// Runs `npm install` and then runs `npm test` class InstallTest extends Install { static description = 'Install package(s) and run tests' static name = 'install-test' @@ -12,4 +11,5 @@ class InstallTest extends Install { return this.npm.exec('test', []) } } + module.exports = InstallTest diff --git a/lib/commands/install.js b/lib/commands/install.js index 6573b4d65555d..24e5f6819b314 100644 --- a/lib/commands/install.js +++ b/lib/commands/install.js @@ -5,8 +5,8 @@ const runScript = require('@npmcli/run-script') const pacote = require('pacote') const checks = require('npm-install-checks') const reifyFinish = require('../utils/reify-finish.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') + class Install extends ArboristWorkspaceCmd { static description = 'Install a package' static name = 'install' @@ -172,4 +172,5 @@ class Install extends ArboristWorkspaceCmd { await reifyFinish(this.npm, arb) } } + module.exports = Install diff --git a/lib/commands/link.js b/lib/commands/link.js index 0442e50dc0d83..bde761c4226dc 100644 --- a/lib/commands/link.js +++ b/lib/commands/link.js @@ -185,4 +185,5 @@ class Link extends ArboristWorkspaceCmd { return missing } } + module.exports = Link diff --git a/lib/commands/login.js b/lib/commands/login.js index d38aec51289cc..0801af9726f78 100644 --- a/lib/commands/login.js +++ b/lib/commands/login.js @@ -1,7 +1,6 @@ const { log, output } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const auth = require('../utils/auth.js') - const BaseCommand = require('../base-command.js') class Login extends BaseCommand { @@ -47,4 +46,5 @@ class Login extends BaseCommand { output.standard(message) } } + module.exports = Login diff --git a/lib/commands/logout.js b/lib/commands/logout.js index 338081cccd457..63a8154cd3fdf 100644 --- a/lib/commands/logout.js +++ b/lib/commands/logout.js @@ -46,4 +46,5 @@ class Logout extends BaseCommand { await this.npm.config.save(level) } } + module.exports = Logout diff --git a/lib/commands/ls.js b/lib/commands/ls.js index cf086924092fe..51e99f429816a 100644 --- a/lib/commands/ls.js +++ b/lib/commands/ls.js @@ -1,10 +1,12 @@ const { resolve, relative, sep } = require('path') -const relativePrefix = `.${sep}` - const archy = require('archy') const { breadth } = require('treeverse') const npa = require('npm-package-arg') const { output } = require('proc-log') +const ArboristWorkspaceCmd = require('../arborist-cmd.js') +const localeCompare = require('@isaacs/string-locale-compare')('en') + +const relativePrefix = `.${sep}` const _depth = Symbol('depth') const _dedupe = Symbol('dedupe') @@ -17,8 +19,6 @@ const _parent = Symbol('parent') const _problems = Symbol('problems') const _required = Symbol('required') const _type = Symbol('type') -const ArboristWorkspaceCmd = require('../arborist-cmd.js') -const localeCompare = require('@isaacs/string-locale-compare')('en') class LS extends ArboristWorkspaceCmd { static description = 'List installed packages' @@ -219,6 +219,7 @@ class LS extends ArboristWorkspaceCmd { return tree } } + module.exports = LS const isGitNode = (node) => { diff --git a/lib/commands/org.js b/lib/commands/org.js index f1e5b0e09c62c..f06c537d3f3f7 100644 --- a/lib/commands/org.js +++ b/lib/commands/org.js @@ -149,4 +149,5 @@ class Org extends BaseCommand { } } } + module.exports = Org diff --git a/lib/commands/outdated.js b/lib/commands/outdated.js index 1600315cac642..ceef86be950c5 100644 --- a/lib/commands/outdated.js +++ b/lib/commands/outdated.js @@ -6,7 +6,6 @@ const npa = require('npm-package-arg') const pickManifest = require('npm-pick-manifest') const { output } = require('proc-log') const localeCompare = require('@isaacs/string-locale-compare')('en') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') class Outdated extends ArboristWorkspaceCmd { @@ -369,4 +368,5 @@ class Outdated extends ArboristWorkspaceCmd { return JSON.stringify(out, null, 2) } } + module.exports = Outdated diff --git a/lib/commands/pack.js b/lib/commands/pack.js index 463329235d417..e679377fa7970 100644 --- a/lib/commands/pack.js +++ b/lib/commands/pack.js @@ -83,4 +83,5 @@ class Pack extends BaseCommand { return this.exec([...this.workspacePaths, ...args.filter(a => a !== '.')]) } } + module.exports = Pack diff --git a/lib/commands/ping.js b/lib/commands/ping.js index 3ae4ed80a22cb..2d8f6f91a9111 100644 --- a/lib/commands/ping.js +++ b/lib/commands/ping.js @@ -26,4 +26,5 @@ class Ping extends BaseCommand { } } } + module.exports = Ping diff --git a/lib/commands/prefix.js b/lib/commands/prefix.js index 6c3d6f886f12c..edd5595d09b41 100644 --- a/lib/commands/prefix.js +++ b/lib/commands/prefix.js @@ -11,4 +11,5 @@ class Prefix extends BaseCommand { return output.standard(this.npm.prefix) } } + module.exports = Prefix diff --git a/lib/commands/profile.js b/lib/commands/profile.js index 8bb19e69fd484..a0676e47889af 100644 --- a/lib/commands/profile.js +++ b/lib/commands/profile.js @@ -1,11 +1,11 @@ -const inspect = require('util').inspect +const { inspect } = require('util') const { URL } = require('url') const { log, output } = require('proc-log') const npmProfile = require('npm-profile') const qrcodeTerminal = require('qrcode-terminal') - const otplease = require('../utils/otplease.js') const readUserInfo = require('../utils/read-user-info.js') +const BaseCommand = require('../base-command.js') const qrcode = url => new Promise((resolve) => qrcodeTerminal.generate(url, resolve)) @@ -33,7 +33,6 @@ const writableProfileKeys = [ 'github', ] -const BaseCommand = require('../base-command.js') class Profile extends BaseCommand { static description = 'Change settings on your registry profile' static name = 'profile' @@ -387,4 +386,5 @@ class Profile extends BaseCommand { } } } + module.exports = Profile diff --git a/lib/commands/prune.js b/lib/commands/prune.js index 189fc29cb8bc3..1bcf8a9576316 100644 --- a/lib/commands/prune.js +++ b/lib/commands/prune.js @@ -1,7 +1,7 @@ -// prune extraneous packages const reifyFinish = require('../utils/reify-finish.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') + +// prune extraneous packages class Prune extends ArboristWorkspaceCmd { static description = 'Remove extraneous packages' static name = 'prune' @@ -30,4 +30,5 @@ class Prune extends ArboristWorkspaceCmd { await reifyFinish(this.npm, arb) } } + module.exports = Prune diff --git a/lib/commands/publish.js b/lib/commands/publish.js index db4cb7f5dc61e..f174d338b6392 100644 --- a/lib/commands/publish.js +++ b/lib/commands/publish.js @@ -7,18 +7,16 @@ const pacote = require('pacote') const npa = require('npm-package-arg') const npmFetch = require('npm-registry-fetch') const { redactLog: replaceInfo } = require('@npmcli/redact') - const otplease = require('../utils/otplease.js') const { getContents, logTar } = require('../utils/tar.js') - // for historical reasons, publishConfig in package.json can contain ANY config // keys that npm supports in .npmrc files and elsewhere. We *may* want to // revisit this at some point, and have a minimal set that's a SemVer-major // change that ought to get a RFC written on it. const { flatten } = require('@npmcli/config/lib/definitions') const pkgJson = require('@npmcli/package-json') - const BaseCommand = require('../base-command.js') + class Publish extends BaseCommand { static description = 'Publish a package' static name = 'publish' @@ -226,4 +224,5 @@ class Publish extends BaseCommand { return manifest } } + module.exports = Publish diff --git a/lib/commands/query.js b/lib/commands/query.js index a25180d6c38d8..1e9dee4ef1fda 100644 --- a/lib/commands/query.js +++ b/lib/commands/query.js @@ -1,5 +1,3 @@ -'use strict' - const { resolve } = require('node:path') const BaseCommand = require('../base-command.js') const { log, output } = require('proc-log') diff --git a/lib/commands/rebuild.js b/lib/commands/rebuild.js index d9e22de7cf048..3894f0aa290cc 100644 --- a/lib/commands/rebuild.js +++ b/lib/commands/rebuild.js @@ -2,8 +2,8 @@ const { resolve } = require('path') const { output } = require('proc-log') const npa = require('npm-package-arg') const semver = require('semver') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') + class Rebuild extends ArboristWorkspaceCmd { static description = 'Rebuild a package' static name = 'rebuild' @@ -80,4 +80,5 @@ class Rebuild extends ArboristWorkspaceCmd { }) } } + module.exports = Rebuild diff --git a/lib/commands/repo.js b/lib/commands/repo.js index b89b74c0bf1ba..8e2fef24771d9 100644 --- a/lib/commands/repo.js +++ b/lib/commands/repo.js @@ -1,6 +1,6 @@ const { URL } = require('url') - const PackageUrlCmd = require('../package-url-cmd.js') + class Repo extends PackageUrlCmd { static description = 'Open package repository page in the browser' static name = 'repo' @@ -30,6 +30,7 @@ class Repo extends PackageUrlCmd { return url } } + module.exports = Repo const unknownHostedUrl = url => { diff --git a/lib/commands/root.js b/lib/commands/root.js index f1f9579d103fd..f7c330a004169 100644 --- a/lib/commands/root.js +++ b/lib/commands/root.js @@ -1,5 +1,6 @@ const { output } = require('proc-log') const BaseCommand = require('../base-command.js') + class Root extends BaseCommand { static description = 'Display npm root' static name = 'root' @@ -9,4 +10,5 @@ class Root extends BaseCommand { output.standard(this.npm.dir) } } + module.exports = Root diff --git a/lib/commands/run-script.js b/lib/commands/run-script.js index 34c0b00fe95b5..f048996e66354 100644 --- a/lib/commands/run-script.js +++ b/lib/commands/run-script.js @@ -1,7 +1,7 @@ const { log, output } = require('proc-log') const pkgJson = require('@npmcli/package-json') - const BaseCommand = require('../base-command.js') + class RunScript extends BaseCommand { static description = 'Run arbitrary package scripts' static params = [ diff --git a/lib/commands/sbom.js b/lib/commands/sbom.js index 4b20bf5ea1f76..4ee21ec1954fd 100644 --- a/lib/commands/sbom.js +++ b/lib/commands/sbom.js @@ -1,5 +1,3 @@ -'use strict' - const localeCompare = require('@isaacs/string-locale-compare')('en') const BaseCommand = require('../base-command.js') const { log, output } = require('proc-log') diff --git a/lib/commands/search.js b/lib/commands/search.js index 2b338ed4d39b8..cffc4cdacc846 100644 --- a/lib/commands/search.js +++ b/lib/commands/search.js @@ -1,7 +1,6 @@ const Pipeline = require('minipass-pipeline') const libSearch = require('libnpmsearch') const { log, output } = require('proc-log') - const formatSearchStream = require('../utils/format-search-stream.js') const BaseCommand = require('../base-command.js') @@ -68,4 +67,5 @@ class Search extends BaseCommand { log.silly('search', 'search completed') } } + module.exports = Search diff --git a/lib/commands/set.js b/lib/commands/set.js index f315d183845c5..509da1956bf1f 100644 --- a/lib/commands/set.js +++ b/lib/commands/set.js @@ -22,4 +22,5 @@ class Set extends BaseCommand { return this.npm.exec('config', ['set'].concat(args)) } } + module.exports = Set diff --git a/lib/commands/shrinkwrap.js b/lib/commands/shrinkwrap.js index 56d7ffa88e99e..008244e9760f3 100644 --- a/lib/commands/shrinkwrap.js +++ b/lib/commands/shrinkwrap.js @@ -69,4 +69,5 @@ class Shrinkwrap extends BaseCommand { } } } + module.exports = Shrinkwrap diff --git a/lib/commands/star.js b/lib/commands/star.js index 39165d8c3d8dc..fbf81e64bf21d 100644 --- a/lib/commands/star.js +++ b/lib/commands/star.js @@ -2,8 +2,8 @@ const fetch = require('npm-registry-fetch') const npa = require('npm-package-arg') const { log, output } = require('proc-log') const getIdentity = require('../utils/get-identity') - const BaseCommand = require('../base-command.js') + class Star extends BaseCommand { static description = 'Mark your favorite packages' static name = 'star' @@ -68,4 +68,5 @@ class Star extends BaseCommand { } } } + module.exports = Star diff --git a/lib/commands/stars.js b/lib/commands/stars.js index 1d92d97d7760a..628b22fdb8808 100644 --- a/lib/commands/stars.js +++ b/lib/commands/stars.js @@ -1,8 +1,8 @@ const fetch = require('npm-registry-fetch') const { log, output } = require('proc-log') const getIdentity = require('../utils/get-identity.js') - const BaseCommand = require('../base-command.js') + class Stars extends BaseCommand { static description = 'View packages marked as favorites' static name = 'stars' @@ -35,4 +35,5 @@ class Stars extends BaseCommand { } } } + module.exports = Stars diff --git a/lib/commands/team.js b/lib/commands/team.js index 4ffaa5ff86ab9..f7bada28a611e 100644 --- a/lib/commands/team.js +++ b/lib/commands/team.js @@ -1,7 +1,6 @@ const columns = require('cli-columns') const libteam = require('libnpmteam') const { output } = require('proc-log') - const otplease = require('../utils/otplease.js') const BaseCommand = require('../base-command.js') @@ -152,4 +151,5 @@ class Team extends BaseCommand { } } } + module.exports = Team diff --git a/lib/commands/token.js b/lib/commands/token.js index 70ff0a332b18a..4429a20319f7e 100644 --- a/lib/commands/token.js +++ b/lib/commands/token.js @@ -1,10 +1,9 @@ const { log, output } = require('proc-log') const profile = require('npm-profile') - const otplease = require('../utils/otplease.js') const readUserInfo = require('../utils/read-user-info.js') - const BaseCommand = require('../base-command.js') + class Token extends BaseCommand { static description = 'Manage your authentication tokens' static name = 'token' @@ -194,4 +193,5 @@ class Token extends BaseCommand { return list } } + module.exports = Token diff --git a/lib/commands/uninstall.js b/lib/commands/uninstall.js index 51df355d5a899..7496c02deb28f 100644 --- a/lib/commands/uninstall.js +++ b/lib/commands/uninstall.js @@ -1,10 +1,9 @@ const { resolve } = require('path') const pkgJson = require('@npmcli/package-json') - const reifyFinish = require('../utils/reify-finish.js') const completion = require('../utils/installed-shallow.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') + class Uninstall extends ArboristWorkspaceCmd { static description = 'Remove a package' static name = 'uninstall' @@ -53,4 +52,5 @@ class Uninstall extends ArboristWorkspaceCmd { await reifyFinish(this.npm, arb) } } + module.exports = Uninstall diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index 3e7e8c8b624a2..e8b7de8395ea9 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -7,12 +7,12 @@ const pkgJson = require('@npmcli/package-json') const { flatten } = require('@npmcli/config/lib/definitions') const getIdentity = require('../utils/get-identity.js') const otplease = require('../utils/otplease.js') +const BaseCommand = require('../base-command.js') const LAST_REMAINING_VERSION_ERROR = 'Refusing to delete the last version of the package. ' + 'It will block from republishing a new version for 24 hours.\n' + 'Run with --force to do this.' -const BaseCommand = require('../base-command.js') class Unpublish extends BaseCommand { static description = 'Remove a package from the registry' static name = 'unpublish' @@ -172,4 +172,5 @@ class Unpublish extends BaseCommand { } } } + module.exports = Unpublish diff --git a/lib/commands/unstar.js b/lib/commands/unstar.js index cbcb73636c638..c72966866669a 100644 --- a/lib/commands/unstar.js +++ b/lib/commands/unstar.js @@ -4,4 +4,5 @@ class Unstar extends Star { static description = 'Remove an item from your favorite packages' static name = 'unstar' } + module.exports = Unstar diff --git a/lib/commands/update.js b/lib/commands/update.js index 63711c05e5b97..ddc3e4a47f38a 100644 --- a/lib/commands/update.js +++ b/lib/commands/update.js @@ -1,10 +1,8 @@ const path = require('path') - const { log } = require('proc-log') - const reifyFinish = require('../utils/reify-finish.js') - const ArboristWorkspaceCmd = require('../arborist-cmd.js') + class Update extends ArboristWorkspaceCmd { static description = 'Update packages' static name = 'update' @@ -66,4 +64,5 @@ class Update extends ArboristWorkspaceCmd { await reifyFinish(this.npm, arb) } } + module.exports = Update diff --git a/lib/commands/version.js b/lib/commands/version.js index 0e1916d7af7fb..4d7b971fa6fbf 100644 --- a/lib/commands/version.js +++ b/lib/commands/version.js @@ -1,7 +1,6 @@ const { resolve } = require('node:path') const { readFile } = require('node:fs/promises') const { output } = require('proc-log') - const BaseCommand = require('../base-command.js') class Version extends BaseCommand { diff --git a/lib/commands/view.js b/lib/commands/view.js index e8b102737a1e9..9c8630bef154e 100644 --- a/lib/commands/view.js +++ b/lib/commands/view.js @@ -9,11 +9,11 @@ const relativeDate = require('tiny-relative-date') const semver = require('semver') const { inspect } = require('util') const { packument } = require('pacote') +const Queryable = require('../utils/queryable.js') +const BaseCommand = require('../base-command.js') const readJson = async file => jsonParse(await readFile(file, 'utf8')) -const Queryable = require('../utils/queryable.js') -const BaseCommand = require('../base-command.js') class View extends BaseCommand { static description = 'View registry info' static name = 'view' @@ -406,6 +406,7 @@ class View extends BaseCommand { } } } + module.exports = View function cleanBlanks (obj) { diff --git a/lib/commands/whoami.js b/lib/commands/whoami.js index e05993abdd5bf..03d7bb24991cf 100644 --- a/lib/commands/whoami.js +++ b/lib/commands/whoami.js @@ -1,7 +1,7 @@ const { output } = require('proc-log') const getIdentity = require('../utils/get-identity.js') - const BaseCommand = require('../base-command.js') + class Whoami extends BaseCommand { static description = 'Display npm username' static name = 'whoami' @@ -14,4 +14,5 @@ class Whoami extends BaseCommand { ) } } + module.exports = Whoami diff --git a/lib/lifecycle-cmd.js b/lib/lifecycle-cmd.js index 848771a38355e..26e04b12af272 100644 --- a/lib/lifecycle-cmd.js +++ b/lib/lifecycle-cmd.js @@ -16,4 +16,5 @@ class LifecycleCmd extends BaseCommand { return this.npm.exec('run-script', [this.constructor.name, ...args]) } } + module.exports = LifecycleCmd diff --git a/lib/npm.js b/lib/npm.js index 509c2fa2abd36..e76317e905ea3 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -445,4 +445,5 @@ class Npm { this.#display.flushOutput(jsonError) } } + module.exports = Npm diff --git a/lib/package-url-cmd.js b/lib/package-url-cmd.js index c81b933f69254..5ab7cb87a2cd3 100644 --- a/lib/package-url-cmd.js +++ b/lib/package-url-cmd.js @@ -62,4 +62,5 @@ class PackageUrlCommand extends BaseCommand { return (rurl && hostedGitInfo.fromUrl(rurl.replace(/^git\+/, ''))) || null } } + module.exports = PackageUrlCommand diff --git a/lib/utils/did-you-mean.js b/lib/utils/did-you-mean.js index ff3c812b46c3c..499c27c74e1de 100644 --- a/lib/utils/did-you-mean.js +++ b/lib/utils/did-you-mean.js @@ -37,4 +37,5 @@ const didYouMean = async (path, scmd) => { : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}` return suggestion } + module.exports = didYouMean diff --git a/lib/utils/verify-signatures.js b/lib/utils/verify-signatures.js new file mode 100644 index 0000000000000..f2973316c9b76 --- /dev/null +++ b/lib/utils/verify-signatures.js @@ -0,0 +1,389 @@ +const fetch = require('npm-registry-fetch') +const localeCompare = require('@isaacs/string-locale-compare')('en') +const npa = require('npm-package-arg') +const pacote = require('pacote') +const pMap = require('p-map') +const tufClient = require('@sigstore/tuf') +const { log, output } = require('proc-log') + +const sortAlphabetically = (a, b) => localeCompare(a.name, b.name) + +class VerifySignatures { + constructor (tree, filterSet, npm, opts) { + this.tree = tree + this.filterSet = filterSet + this.npm = npm + this.opts = opts + this.keys = new Map() + this.invalid = [] + this.missing = [] + this.checkedPackages = new Set() + this.auditedWithKeysCount = 0 + this.verifiedSignatureCount = 0 + this.verifiedAttestationCount = 0 + this.exitCode = 0 + } + + async run () { + const start = process.hrtime.bigint() + + // Find all deps in tree + const { edges, registries } = this.getEdgesOut(this.tree.inventory.values(), this.filterSet) + if (edges.size === 0) { + throw new Error('found no installed dependencies to audit') + } + + const tuf = await tufClient.initTUF({ + cachePath: this.opts.tufCache, + retry: this.opts.retry, + timeout: this.opts.timeout, + }) + await Promise.all([...registries].map(registry => this.setKeys({ registry, tuf }))) + + log.verbose('verifying registry signatures') + await pMap(edges, (e) => this.getVerifiedInfo(e), { concurrency: 20, stopOnError: true }) + + // Didn't find any dependencies that could be verified, e.g. only local + // deps, missing version, not on a registry etc. + if (!this.auditedWithKeysCount) { + throw new Error('found no dependencies to audit that were installed from ' + + 'a supported registry') + } + + const invalid = this.invalid.sort(sortAlphabetically) + const missing = this.missing.sort(sortAlphabetically) + + const hasNoInvalidOrMissing = invalid.length === 0 && missing.length === 0 + + if (!hasNoInvalidOrMissing) { + process.exitCode = 1 + } + + if (this.npm.config.get('json')) { + output.standard(JSON.stringify({ + invalid, + missing, + }, null, 2)) + return + } + const end = process.hrtime.bigint() + const elapsed = end - start + + const auditedPlural = this.auditedWithKeysCount > 1 ? 's' : '' + const timing = `audited ${this.auditedWithKeysCount} package${auditedPlural} in ` + + `${Math.floor(Number(elapsed) / 1e9)}s` + output.standard(timing) + output.standard('') + + const verifiedBold = this.npm.chalk.bold('verified') + if (this.verifiedSignatureCount) { + if (this.verifiedSignatureCount === 1) { + /* eslint-disable-next-line max-len */ + output.standard(`${this.verifiedSignatureCount} package has a ${verifiedBold} registry signature`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`${this.verifiedSignatureCount} packages have ${verifiedBold} registry signatures`) + } + output.standard('') + } + + if (this.verifiedAttestationCount) { + if (this.verifiedAttestationCount === 1) { + /* eslint-disable-next-line max-len */ + output.standard(`${this.verifiedAttestationCount} package has a ${verifiedBold} attestation`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`${this.verifiedAttestationCount} packages have ${verifiedBold} attestations`) + } + output.standard('') + } + + if (missing.length) { + const missingClr = this.npm.chalk.redBright('missing') + if (missing.length === 1) { + /* eslint-disable-next-line max-len */ + output.standard(`1 package has a ${missingClr} registry signature but the registry is providing signing keys:`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`${missing.length} packages have ${missingClr} registry signatures but the registry is providing signing keys:`) + } + output.standard('') + missing.map(m => + output.standard(`${this.npm.chalk.red(`${m.name}@${m.version}`)} (${m.registry})`) + ) + } + + if (invalid.length) { + if (missing.length) { + output.standard('') + } + const invalidClr = this.npm.chalk.redBright('invalid') + // We can have either invalid signatures or invalid provenance + const invalidSignatures = this.invalid.filter(i => i.code === 'EINTEGRITYSIGNATURE') + if (invalidSignatures.length) { + if (invalidSignatures.length === 1) { + output.standard(`1 package has an ${invalidClr} registry signature:`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`${invalidSignatures.length} packages have ${invalidClr} registry signatures:`) + } + output.standard('') + invalidSignatures.map(i => + output.standard(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`) + ) + output.standard('') + } + + const invalidAttestations = this.invalid.filter(i => i.code === 'EATTESTATIONVERIFY') + if (invalidAttestations.length) { + if (invalidAttestations.length === 1) { + output.standard(`1 package has an ${invalidClr} attestation:`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`${invalidAttestations.length} packages have ${invalidClr} attestations:`) + } + output.standard('') + invalidAttestations.map(i => + output.standard(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`) + ) + output.standard('') + } + + if (invalid.length === 1) { + /* eslint-disable-next-line max-len */ + output.standard(`Someone might have tampered with this package since it was published on the registry!`) + } else { + /* eslint-disable-next-line max-len */ + output.standard(`Someone might have tampered with these packages since they were published on the registry!`) + } + output.standard('') + } + } + + getEdgesOut (nodes, filterSet) { + const edges = new Set() + const registries = new Set() + for (const node of nodes) { + for (const edge of node.edgesOut.values()) { + const filteredOut = + edge.from + && filterSet + && filterSet.size > 0 + && !filterSet.has(edge.from.target) + + if (!filteredOut) { + const spec = this.getEdgeSpec(edge) + if (spec) { + // Prefetch and cache public keys from used registries + registries.add(this.getSpecRegistry(spec)) + } + edges.add(edge) + } + } + } + return { edges, registries } + } + + async setKeys ({ registry, tuf }) { + const { host, pathname } = new URL(registry) + // Strip any trailing slashes from pathname + const regKey = `${host}${pathname.replace(/\/$/, '')}/keys.json` + let keys = await tuf.getTarget(regKey) + .then((target) => JSON.parse(target)) + .then(({ keys: ks }) => ks.map((key) => ({ + ...key, + keyid: key.keyId, + pemkey: `-----BEGIN PUBLIC KEY-----\n${key.publicKey.rawBytes}\n-----END PUBLIC KEY-----`, + expires: key.publicKey.validFor.end || null, + }))).catch(err => { + if (err.code === 'TUF_FIND_TARGET_ERROR') { + return null + } else { + throw err + } + }) + + // If keys not found in Sigstore TUF repo, fallback to registry keys API + if (!keys) { + keys = await fetch.json('/-/npm/v1/keys', { + ...this.npm.flatOptions, + registry, + }).then(({ keys: ks }) => ks.map((key) => ({ + ...key, + pemkey: `-----BEGIN PUBLIC KEY-----\n${key.key}\n-----END PUBLIC KEY-----`, + }))).catch(err => { + if (err.code === 'E404' || err.code === 'E400') { + return null + } else { + throw err + } + }) + } + + if (keys) { + this.keys.set(registry, keys) + } + } + + getEdgeType (edge) { + return edge.optional ? 'optionalDependencies' + : edge.peer ? 'peerDependencies' + : edge.dev ? 'devDependencies' + : 'dependencies' + } + + getEdgeSpec (edge) { + let name = edge.name + try { + name = npa(edge.spec).subSpec.name + } catch { + // leave it as edge.name + } + try { + return npa(`${name}@${edge.spec}`) + } catch { + // Skip packages with invalid spec + } + } + + buildRegistryConfig (registry) { + const keys = this.keys.get(registry) || [] + const parsedRegistry = new URL(registry) + const regKey = `//${parsedRegistry.host}${parsedRegistry.pathname}` + return { + [`${regKey}:_keys`]: keys, + } + } + + getSpecRegistry (spec) { + return fetch.pickRegistry(spec, this.npm.flatOptions) + } + + getValidPackageInfo (edge) { + const type = this.getEdgeType(edge) + // Skip potentially optional packages that are not on disk, as these could + // be omitted during install + if (edge.error === 'MISSING' && type !== 'dependencies') { + return + } + + const spec = this.getEdgeSpec(edge) + // Skip invalid version requirements + if (!spec) { + return + } + const node = edge.to || edge + const { version } = node.package || {} + + if (node.isWorkspace || // Skip local workspaces packages + !version || // Skip packages that don't have a installed version, e.g. optonal dependencies + !spec.registry) { // Skip if not from registry, e.g. git package + return + } + + for (const omitType of this.npm.config.get('omit')) { + if (node[omitType]) { + return + } + } + + return { + name: spec.name, + version, + type, + location: node.location, + registry: this.getSpecRegistry(spec), + } + } + + async verifySignatures (name, version, registry) { + const { + _integrity: integrity, + _signatures, + _attestations, + _resolved: resolved, + } = await pacote.manifest(`${name}@${version}`, { + verifySignatures: true, + verifyAttestations: true, + ...this.buildRegistryConfig(registry), + ...this.npm.flatOptions, + }) + const signatures = _signatures || [] + const result = { + integrity, + signatures, + attestations: _attestations, + resolved, + } + return result + } + + async getVerifiedInfo (edge) { + const info = this.getValidPackageInfo(edge) + if (!info) { + return + } + const { name, version, location, registry, type } = info + if (this.checkedPackages.has(location)) { + // we already did or are doing this one + return + } + this.checkedPackages.add(location) + + // We only "audit" or verify the signature, or the presence of it, on + // packages whose registry returns signing keys + const keys = this.keys.get(registry) || [] + if (keys.length) { + this.auditedWithKeysCount += 1 + } + + try { + const { integrity, signatures, attestations, resolved } = await this.verifySignatures( + name, version, registry + ) + + // Currently we only care about missing signatures on registries that provide a public key + // We could make this configurable in the future with a strict/paranoid mode + if (signatures.length) { + this.verifiedSignatureCount += 1 + } else if (keys.length) { + this.missing.push({ + integrity, + location, + name, + registry, + resolved, + version, + }) + } + + // Track verified attestations separately to registry signatures, as all + // packages on registries with signing keys are expected to have registry + // signatures, but not all packages have provenance and publish attestations. + if (attestations) { + this.verifiedAttestationCount += 1 + } + } catch (e) { + if (e.code === 'EINTEGRITYSIGNATURE' || e.code === 'EATTESTATIONVERIFY') { + this.invalid.push({ + code: e.code, + message: e.message, + integrity: e.integrity, + keyid: e.keyid, + location, + name, + registry, + resolved: e.resolved, + signature: e.signature, + predicateType: e.predicateType, + type, + version, + }) + } else { + throw e + } + } + } +} + +module.exports = VerifySignatures From 55665dd4e4516366d38d0870b0c47ca476ec7536 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 14:45:45 -0700 Subject: [PATCH 03/16] fix(cleanup): move cli specific files to separate dir --- .eslintrc.local.js | 4 +-- lib/cli.js | 4 +-- lib/{cli-entry.js => cli/entry.js} | 27 ++++++++++++++-- lib/{utils => cli}/exit-handler.js | 2 +- lib/{utils => cli}/update-notifier.js | 0 lib/{es6 => cli}/validate-engines.js | 0 lib/npm.js | 31 +------------------ smoke-tests/test/npm-replace-global.js | 2 +- .../update-notifier.js.test.cjs | 24 +++++++------- test/fixtures/mock-npm.js | 6 +--- test/lib/cli.js | 2 +- test/lib/{cli-entry.js => cli/entry.js} | 23 +++++++++++--- test/lib/{utils => cli}/update-notifier.js | 2 +- test/lib/{es6 => cli}/validate-engines.js | 2 +- test/lib/npm.js | 5 ++- test/lib/utils/exit-handler.js | 2 +- test/lib/utils/installed-deep.js | 2 +- test/lib/utils/installed-shallow.js | 2 +- 18 files changed, 70 insertions(+), 70 deletions(-) rename lib/{cli-entry.js => cli/entry.js} (71%) rename lib/{utils => cli}/exit-handler.js (99%) rename lib/{utils => cli}/update-notifier.js (100%) rename lib/{es6 => cli}/validate-engines.js (100%) rename tap-snapshots/test/lib/{utils => cli}/update-notifier.js.test.cjs (64%) rename test/lib/{cli-entry.js => cli/entry.js} (88%) rename test/lib/{utils => cli}/update-notifier.js (99%) rename test/lib/{es6 => cli}/validate-engines.js (94%) diff --git a/.eslintrc.local.js b/.eslintrc.local.js index 6d94f64614fb8..2dce9d2badc08 100644 --- a/.eslintrc.local.js +++ b/.eslintrc.local.js @@ -25,8 +25,8 @@ module.exports = { overrides: es6Files({ 'index.js': ['lib/cli.js'], 'bin/npm-cli.js': ['lib/cli.js'], - 'lib/cli.js': ['lib/es6/validate-engines.js'], - 'lib/es6/validate-engines.js': ['package.json'], + 'lib/cli.js': ['lib/cli/validate-engines.js'], + 'lib/cli/validate-engines.js': ['package.json'], // TODO: This file should also have its requires restricted as well since it // is an entry point but it currently pulls in config definitions which have // a large require graph, so that is not currently feasible. A future config diff --git a/lib/cli.js b/lib/cli.js index 84d5471ad5e91..e11729fe3205b 100644 --- a/lib/cli.js +++ b/lib/cli.js @@ -1,4 +1,4 @@ -const validateEngines = require('./es6/validate-engines.js') -const cliEntry = require('node:path').resolve(__dirname, 'cli-entry.js') +const validateEngines = require('./cli/validate-engines.js') +const cliEntry = require('node:path').resolve(__dirname, 'cli/entry.js') module.exports = (process) => validateEngines(process, () => require(cliEntry)) diff --git a/lib/cli-entry.js b/lib/cli/entry.js similarity index 71% rename from lib/cli-entry.js rename to lib/cli/entry.js index 9a5994aec19a3..1c5f7e0dc1545 100644 --- a/lib/cli-entry.js +++ b/lib/cli/entry.js @@ -11,9 +11,12 @@ module.exports = async (process, validateEngines) => { process.argv.splice(1, 1, 'npm', '-g') } + // Patch the global fs module here at the app level + require('graceful-fs').gracefulify(require('fs')) + const satisfies = require('semver/functions/satisfies') - const exitHandler = require('./utils/exit-handler.js') - const Npm = require('./npm.js') + const exitHandler = require('./exit-handler.js') + const Npm = require('../npm.js') const npm = new Npm() exitHandler.setNpm(npm) @@ -58,11 +61,29 @@ module.exports = async (process, validateEngines) => { return exitHandler() } + // this is async but we dont await it, since its ok if it doesnt + // finish before the command finishes running. it uses command and argv + // so it must be initiated here, after the command name is set + const updateNotifier = require('./update-notifier.js') + // eslint-disable-next-line promise/catch-or-return + updateNotifier(npm).then((msg) => (npm.updateNotification = msg)) + + // Options are prefixed by a hyphen-minus (-, \u2d). + // Other dash-type chars look similar but are invalid. + const nonDashArgs = npm.argv.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a)) + if (nonDashArgs.length) { + log.error( + 'arg', + 'Argument starts with non-ascii dash, this is probably invalid:', + nonDashArgs.join(', ') + ) + } + await npm.exec(cmd) return exitHandler() } catch (err) { if (err.code === 'EUNKNOWNCOMMAND') { - const didYouMean = require('./utils/did-you-mean.js') + const didYouMean = require('../utils/did-you-mean.js') const suggestions = await didYouMean(npm.localPrefix, cmd) output.standard(`Unknown command: "${cmd}"${suggestions}\n`) output.standard('To see a list of supported npm commands, run:\n npm help') diff --git a/lib/utils/exit-handler.js b/lib/cli/exit-handler.js similarity index 99% rename from lib/utils/exit-handler.js rename to lib/cli/exit-handler.js index 76d7d6036e072..6dc44de1f003a 100644 --- a/lib/utils/exit-handler.js +++ b/lib/cli/exit-handler.js @@ -1,5 +1,5 @@ const { log, output, time } = require('proc-log') -const errorMessage = require('./error-message.js') +const errorMessage = require('../utils/error-message.js') const { redactLog: replaceInfo } = require('@npmcli/redact') let npm = null // set by the cli diff --git a/lib/utils/update-notifier.js b/lib/cli/update-notifier.js similarity index 100% rename from lib/utils/update-notifier.js rename to lib/cli/update-notifier.js diff --git a/lib/es6/validate-engines.js b/lib/cli/validate-engines.js similarity index 100% rename from lib/es6/validate-engines.js rename to lib/cli/validate-engines.js diff --git a/lib/npm.js b/lib/npm.js index e76317e905ea3..d52f8bdc1c480 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -2,10 +2,6 @@ const { resolve, dirname, join } = require('node:path') const Config = require('@npmcli/config') const which = require('which') const fs = require('node:fs/promises') - -// Patch the global fs module here at the app level -require('graceful-fs').gracefulify(require('fs')) - const { definitions, flatten, shorthands } = require('@npmcli/config/lib/definitions') const usage = require('./utils/npm-usage.js') const LogFile = require('./utils/log-file.js') @@ -13,7 +9,6 @@ const Timers = require('./utils/timers.js') const Display = require('./utils/display.js') const { log, time } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') -const updateNotifier = require('./utils/update-notifier.js') const pkg = require('../package.json') const { deref } = require('./utils/cmd-list.js') @@ -42,7 +37,6 @@ class Npm { #title = 'npm' #argvClean = [] #npmRoot = null - #warnedNonDashArg = false #chalk = null #logChalk = null @@ -109,30 +103,7 @@ class Npm { // passed in directly. async exec (cmd, args = this.argv) { const command = this.setCmd(cmd) - - const timeEnd = time.start(`command:${cmd}`) - - // this is async but we dont await it, since its ok if it doesnt - // finish before the command finishes running. it uses command and argv - // so it must be initiated here, after the command name is set - // eslint-disable-next-line promise/catch-or-return - updateNotifier(this).then((msg) => (this.updateNotification = msg)) - - // Options are prefixed by a hyphen-minus (-, \u2d). - // Other dash-type chars look similar but are invalid. - if (!this.#warnedNonDashArg) { - const nonDashArgs = args.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a)) - if (nonDashArgs.length) { - this.#warnedNonDashArg = true - log.error( - 'arg', - 'Argument starts with non-ascii dash, this is probably invalid:', - nonDashArgs.join(', ') - ) - } - } - - return command.cmdExec(args).finally(timeEnd) + return time.start(`command:${cmd}`, () => command.cmdExec(args)) } async load () { diff --git a/smoke-tests/test/npm-replace-global.js b/smoke-tests/test/npm-replace-global.js index 5b84daf356ce2..c0accf6ba53b5 100644 --- a/smoke-tests/test/npm-replace-global.js +++ b/smoke-tests/test/npm-replace-global.js @@ -190,7 +190,7 @@ t.test('fail when updating with lazy require', async t => { // exit-handler is the last thing called in the code // so an uncached lazy require within the exit handler will always throw await fs.writeFile( - join(paths.globalNodeModules, 'npm/lib/utils/exit-handler.js'), + join(paths.globalNodeModules, 'npm/lib/cli/exit-handler.js'), `module.exports = () => require('./LAZY_REQUIRE_CANARY');module.exports.setNpm = () => {}`, 'utf-8' ) diff --git a/tap-snapshots/test/lib/utils/update-notifier.js.test.cjs b/tap-snapshots/test/lib/cli/update-notifier.js.test.cjs similarity index 64% rename from tap-snapshots/test/lib/utils/update-notifier.js.test.cjs rename to tap-snapshots/test/lib/cli/update-notifier.js.test.cjs index 5693b3f3a9097..244d5216340f8 100644 --- a/tap-snapshots/test/lib/utils/update-notifier.js.test.cjs +++ b/tap-snapshots/test/lib/cli/update-notifier.js.test.cjs @@ -5,7 +5,7 @@ * Make sure to inspect the output below. Do not ignore changes! */ 'use strict' -exports[`test/lib/utils/update-notifier.js TAP notification situations 122.420.69 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 122.420.69 - color=always > must match snapshot 1`] = ` New major version of npm available! 122.420.69 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -13,7 +13,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 122.420.69 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 122.420.69 - color=false > must match snapshot 1`] = ` New major version of npm available! 122.420.69 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -21,7 +21,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.419.69 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.419.69 - color=always > must match snapshot 1`] = ` New minor version of npm available! 123.419.69 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -29,7 +29,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.419.69 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.419.69 - color=false > must match snapshot 1`] = ` New minor version of npm available! 123.419.69 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -37,7 +37,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.68 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.68 - color=always > must match snapshot 1`] = ` New patch version of npm available! 123.420.68 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -45,7 +45,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.68 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.68 - color=false > must match snapshot 1`] = ` New patch version of npm available! 123.420.68 -> 123.420.69 Changelog: https://github.com/npm/cli/releases/tag/v123.420.69 @@ -53,7 +53,7 @@ To update run: npm install -g npm@123.420.69 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.70 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.70 - color=always > must match snapshot 1`] = ` New minor version of npm available! 123.420.70 -> 123.421.70 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70 @@ -61,7 +61,7 @@ To update run: npm install -g npm@123.421.70 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.420.70 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.420.70 - color=false > must match snapshot 1`] = ` New minor version of npm available! 123.420.70 -> 123.421.70 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70 @@ -69,7 +69,7 @@ To update run: npm install -g npm@123.421.70 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.421.69 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.421.69 - color=always > must match snapshot 1`] = ` New patch version of npm available! 123.421.69 -> 123.421.70 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70 @@ -77,7 +77,7 @@ To update run: npm install -g npm@123.421.70 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 123.421.69 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 123.421.69 - color=false > must match snapshot 1`] = ` New patch version of npm available! 123.421.69 -> 123.421.70 Changelog: https://github.com/npm/cli/releases/tag/v123.421.70 @@ -85,7 +85,7 @@ To update run: npm install -g npm@123.421.70 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=always > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=always > must match snapshot 1`] = ` New prerelease version of npm available! 124.0.0-beta.0 -> 124.0.0-beta.99999 Changelog: https://github.com/npm/cli/releases/tag/v124.0.0-beta.99999 @@ -93,7 +93,7 @@ To update run: npm install -g npm@124.0.0-beta.99999 ` -exports[`test/lib/utils/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=false > must match snapshot 1`] = ` +exports[`test/lib/cli/update-notifier.js TAP notification situations 124.0.0-beta.0 - color=false > must match snapshot 1`] = ` New prerelease version of npm available! 124.0.0-beta.0 -> 124.0.0-beta.99999 Changelog: https://github.com/npm/cli/releases/tag/v124.0.0-beta.99999 diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js index 6bb3123d2b091..466b9b2a0ba46 100644 --- a/test/fixtures/mock-npm.js +++ b/test/fixtures/mock-npm.js @@ -48,17 +48,13 @@ const setGlobalNodeModules = (globalDir) => { } const buildMocks = (t, mocks) => { - const allMocks = { - '{LIB}/utils/update-notifier.js': async () => {}, - ...mocks, - } + const allMocks = { ...mocks } // The definitions must be mocked since they are a singleton that reads from // process and environs to build defaults in order to break the requiure // cache. We also need to mock them with any mocks that were passed in for the // test in case those mocks are for things like ci-info which is used there. const definitions = '@npmcli/config/lib/definitions' allMocks[definitions] = tmock(t, definitions, allMocks) - return allMocks } diff --git a/test/lib/cli.js b/test/lib/cli.js index a6cb576e886ee..6c3079b8efe31 100644 --- a/test/lib/cli.js +++ b/test/lib/cli.js @@ -3,7 +3,7 @@ const tmock = require('../fixtures/tmock') t.test('returns cli-entry function', async t => { const cli = tmock(t, '{LIB}/cli.js', { - '{LIB}/cli-entry.js': () => 'ENTRY', + '{LIB}/cli/entry.js': () => 'ENTRY', }) t.equal(cli(process), 'ENTRY') diff --git a/test/lib/cli-entry.js b/test/lib/cli/entry.js similarity index 88% rename from test/lib/cli-entry.js rename to test/lib/cli/entry.js index 3a8d3321c4b64..66a417e7d269a 100644 --- a/test/lib/cli-entry.js +++ b/test/lib/cli/entry.js @@ -1,7 +1,7 @@ const t = require('tap') -const { load: loadMockNpm } = require('../fixtures/mock-npm.js') -const tmock = require('../fixtures/tmock.js') -const validateEngines = require('../../lib/es6/validate-engines.js') +const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') +const tmock = require('../../fixtures/tmock.js') +const validateEngines = require('../../../lib/cli/validate-engines.js') const cliMock = async (t, opts) => { let exitHandlerArgs = null @@ -13,9 +13,9 @@ const cliMock = async (t, opts) => { exitHandlerMock.setNpm = _npm => npm = _npm const { Npm, ...mock } = await loadMockNpm(t, { ...opts, init: false }) - const cli = tmock(t, '{LIB}/cli-entry.js', { + const cli = tmock(t, '{LIB}/cli/entry.js', { '{LIB}/npm.js': Npm, - '{LIB}/utils/exit-handler.js': exitHandlerMock, + '{LIB}/cli/exit-handler.js': exitHandlerMock, }) return { @@ -159,3 +159,16 @@ t.test('unsupported node version', async t => { /npm v.* does not support Node\.js 12\.6\.0\./ ) }) + +t.test('non-ascii dash', async t => { + const { cli, logs } = await cliMock(t, { + globals: { + 'process.argv': ['node', 'npm', 'scope', '\u2010not-a-dash'], + }, + }) + await cli(process) + t.equal( + logs.error[0], + 'arg Argument starts with non-ascii dash, this is probably invalid: \u2010not-a-dash' + ) +}) diff --git a/test/lib/utils/update-notifier.js b/test/lib/cli/update-notifier.js similarity index 99% rename from test/lib/utils/update-notifier.js rename to test/lib/cli/update-notifier.js index 052367c60dadb..2d29868b792a1 100644 --- a/test/lib/utils/update-notifier.js +++ b/test/lib/cli/update-notifier.js @@ -83,7 +83,7 @@ const runUpdateNotifier = async (t, { prefixDir, argv, }) - const updateNotifier = tmock(t, '{LIB}/utils/update-notifier.js', mocks) + const updateNotifier = tmock(t, '{LIB}/cli/update-notifier.js', mocks) const result = await updateNotifier(mock.npm) diff --git a/test/lib/es6/validate-engines.js b/test/lib/cli/validate-engines.js similarity index 94% rename from test/lib/es6/validate-engines.js rename to test/lib/cli/validate-engines.js index 0e6bce726af96..1c0b59700a773 100644 --- a/test/lib/es6/validate-engines.js +++ b/test/lib/cli/validate-engines.js @@ -3,7 +3,7 @@ const mockGlobals = require('@npmcli/mock-globals') const tmock = require('../../fixtures/tmock') const mockValidateEngines = (t) => { - const validateEngines = tmock(t, '{LIB}/es6/validate-engines.js', { + const validateEngines = tmock(t, '{LIB}/cli/validate-engines.js', { '{ROOT}/package.json': { version: '1.2.3', engines: { node: '>=0' } }, }) mockGlobals(t, { 'process.version': 'v4.5.6' }) diff --git a/test/lib/npm.js b/test/lib/npm.js index a0dec04caa137..bc9042365abea 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -178,17 +178,16 @@ t.test('npm.load', async t => { outputs.length = 0 logs.length = 0 - await npm.exec('get', ['scope', '\u2010not-a-dash']) + await npm.exec('get', ['scope', 'usage']) t.strictSame([npm.command, npm.flatOptions.npmCommand], ['ll', 'll'], 'does not change npm.command when another command is called') t.match(logs, [ - 'error arg Argument starts with non-ascii dash, this is probably invalid: \u2010not-a-dash', /timing command:config Completed in [0-9.]+ms/, /timing command:get Completed in [0-9.]+ms/, ]) - t.same(outputs, ['scope=@foo\n\u2010not-a-dash=undefined']) + t.same(outputs, ['scope=@foo\nusage=false']) }) await t.test('--no-workspaces with --workspace', async t => { diff --git a/test/lib/utils/exit-handler.js b/test/lib/utils/exit-handler.js index af92611de0594..b1f4d9c272533 100644 --- a/test/lib/utils/exit-handler.js +++ b/test/lib/utils/exit-handler.js @@ -72,7 +72,7 @@ const mockExitHandler = async (t, { config, mocks, files, ...opts } = {}) => { }, }) - const exitHandler = tmock(t, '{LIB}/utils/exit-handler.js', { + const exitHandler = tmock(t, '{LIB}/cli/exit-handler.js', { '{LIB}/utils/error-message.js': (err) => ({ summary: [['ERR SUMMARY', err.message]], detail: [['ERR DETAIL', err.message]], diff --git a/test/lib/utils/installed-deep.js b/test/lib/utils/installed-deep.js index 4d212746cba89..20e001aaec751 100644 --- a/test/lib/utils/installed-deep.js +++ b/test/lib/utils/installed-deep.js @@ -1,6 +1,6 @@ const t = require('tap') const installedDeep = require('../../../lib/utils/installed-deep.js') -const mockNpm = require('../../../fixtures/mock-npm') +const mockNpm = require('../../fixtures/mock-npm') const fixture = { 'package.json': JSON.stringify({ diff --git a/test/lib/utils/installed-shallow.js b/test/lib/utils/installed-shallow.js index 43ccda4daea69..67b49292a64c7 100644 --- a/test/lib/utils/installed-shallow.js +++ b/test/lib/utils/installed-shallow.js @@ -1,6 +1,6 @@ const t = require('tap') const installed = require('../../../lib/utils/installed-shallow.js') -const mockNpm = require('../../../fixtures/mock-npm') +const mockNpm = require('../../fixtures/mock-npm') const mockShallow = async (t, config) => { const res = await mockNpm(t, { From 06ca31f9e68a532710eaffab3b90fab2db886fc1 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 15:12:25 -0700 Subject: [PATCH 04/16] fix: remove granular config timers --- workspaces/config/lib/index.js | 26 +++++++--------- workspaces/config/test/index.js | 54 +++++++++++++++++++-------------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index c3e73dc761c57..49c4f0a18115c 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -233,27 +233,24 @@ class Config { throw new Error('attempting to load npm config multiple times') } - const timeEnd = time.start('config:load') - // first load the defaults, which sets the global prefix - time.start('config:load:defaults', () => this.loadDefaults()) + this.loadDefaults() // next load the builtin config, as this sets new effective defaults - await time.start('config:load:builtin', () => this.loadBuiltinConfig()) + await this.loadBuiltinConfig() // cli and env are not async, and can set the prefix, relevant to project - time.start('config:load:cli', () => this.loadCLI()) - - time.start('config:load:env', () => this.loadEnv()) + this.loadCLI() + this.loadEnv() // next project config, which can affect userconfig location - await time.start('config:load:project', () => this.loadProjectConfig()) + await this.loadProjectConfig() // then user config, which can affect globalconfig location - await time.start('config:load:user', () => this.loadUserConfig()) + await this.loadUserConfig() // last but not least, global config file - await time.start('config:load:global', () => this.loadGlobalConfig()) + await this.loadGlobalConfig() // set this before calling setEnvs, so that we don't have to share // private attributes, as that module also does a bunch of get operations @@ -262,9 +259,7 @@ class Config { // set proper globalPrefix now that everything is loaded this.globalPrefix = this.get('prefix') - time.start('config:load:setEnvs', () => this.setEnvs()) - - timeEnd() + this.setEnvs() } loadDefaults () { @@ -590,7 +585,8 @@ class Config { async #loadFile (file, type) { // only catch the error from readFile, not from the loadObject call - await time.start(`config:load:file:${file}`, () => readFile(file, 'utf8').then( + log.silly(`config:load:file:${file}`) + await readFile(file, 'utf8').then( data => { const parsedConfig = ini.parse(data) if (type === 'project' && parsedConfig.prefix) { @@ -601,7 +597,7 @@ class Config { return this.#loadObject(parsedConfig, type, file) }, er => this.#loadObject(null, type, file, er) - )) + ) } loadBuiltinConfig () { diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index 9e9fac4f6dc34..e1c976e2e52d0 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -237,9 +237,10 @@ loglevel = yolo }) logs.length = 0 await config.load() - t.match(logs, [['verbose', 'config', 'error loading user config', { - message: 'weird error', - }]]) + t.match(logs.find(l => l[0] === 'verbose'), + ['verbose', 'config', 'error loading user config', { + message: 'weird error', + }]) logs.length = 0 }) @@ -375,7 +376,7 @@ loglevel = yolo // warn logs are emitted as a side effect of validate config.validate() - t.strictSame(logs, [ + t.strictSame(logs.filter(l => l[0] === 'warn'), [ ['warn', 'invalid config', 'registry="hello"', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'full url with "http://"'], ['warn', 'invalid config', 'proxy="hello"', 'set in command line options'], @@ -392,8 +393,7 @@ loglevel = yolo ['warn', 'invalid config', 'prefix=true', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'valid filesystem path'], ['warn', 'config', 'also', 'Please use --include=dev instead.'], - ['warn', 'invalid config', 'loglevel="yolo"', - `set in ${resolve(path, 'project/.npmrc')}`], + ['warn', 'invalid config', 'loglevel="yolo"', `set in ${resolve(path, 'project/.npmrc')}`], ['warn', 'invalid config', 'Must be one of:', ['silent', 'error', 'warn', 'notice', 'http', 'info', 'verbose', 'silly'].join(', '), ], @@ -570,7 +570,7 @@ loglevel = yolo all: config.get('all'), }) - t.strictSame(logs, [ + t.strictSame(logs.filter(l => l[0] === 'warn'), [ ['warn', 'invalid config', 'registry="hello"', 'set in command line options'], ['warn', 'invalid config', 'Must be', 'full url with "http://"'], ['warn', 'invalid config', 'proxy="hello"', 'set in command line options'], @@ -1183,8 +1183,9 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, path, 'localPrefix is the root') t.same(config.get('workspace'), [join(path, 'workspaces', 'one')], 'set the workspace') - t.equal(logs.length, 1, 'got one log message') - t.match(logs[0], ['info', /^found workspace root at/], 'logged info about workspace root') + const info = logs.filter(l => l[0] === 'info') + t.equal(info.length, 1, 'got one log message') + t.match(info[0], ['info', /^found workspace root at/], 'logged info about workspace root') }) t.test('finds other workspace parent', async (t) => { @@ -1204,8 +1205,9 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, path, 'localPrefix is the root') t.same(config.get('workspace'), ['../two'], 'kept the specified workspace') - t.equal(logs.length, 1, 'got one log message') - t.match(logs[0], ['info', /^found workspace root at/], 'logged info about workspace root') + const info = logs.filter(l => l[0] === 'info') + t.equal(info.length, 1, 'got one log message') + t.match(info[0], ['info', /^found workspace root at/], 'logged info about workspace root') }) t.test('warns when workspace has .npmrc', async (t) => { @@ -1225,9 +1227,10 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, path, 'localPrefix is the root') t.same(config.get('workspace'), [join(path, 'workspaces', 'three')], 'kept the workspace') - t.equal(logs.length, 2, 'got two log messages') - t.match(logs[0], ['warn', /^ignoring workspace config/], 'warned about ignored config') - t.match(logs[1], ['info', /^found workspace root at/], 'logged info about workspace root') + const filtered = logs.filter(l => l[0] === 'info' || l[0] === 'warn') + t.equal(filtered.length, 2, 'got two log messages') + t.match(filtered[0], ['warn', /^ignoring workspace config/], 'warned about ignored config') + t.match(filtered[1], ['info', /^found workspace root at/], 'logged info about workspace root') }) t.test('prefix skips auto detect', async (t) => { @@ -1247,7 +1250,8 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, join(path, 'workspaces', 'one'), 'localPrefix is the root') t.same(config.get('workspace'), [], 'did not set workspace') - t.equal(logs.length, 0, 'got no log messages') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 0, 'got no log messages') }) t.test('no-workspaces skips auto detect', async (t) => { @@ -1267,7 +1271,8 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, join(path, 'workspaces', 'one'), 'localPrefix is the root') t.same(config.get('workspace'), [], 'did not set workspace') - t.equal(logs.length, 0, 'got no log messages') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 0, 'got no log messages') }) t.test('global skips auto detect', async (t) => { @@ -1287,7 +1292,8 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, join(path, 'workspaces', 'one'), 'localPrefix is the root') t.same(config.get('workspace'), [], 'did not set workspace') - t.equal(logs.length, 0, 'got no log messages') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 0, 'got no log messages') }) t.test('location=global skips auto detect', async (t) => { @@ -1307,7 +1313,8 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, join(path, 'workspaces', 'one'), 'localPrefix is the root') t.same(config.get('workspace'), [], 'did not set workspace') - t.equal(logs.length, 0, 'got no log messages') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 0, 'got no log messages') }) t.test('excludeNpmCwd skips auto detect', async (t) => { @@ -1328,7 +1335,8 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, join(path, 'workspaces', 'one'), 'localPrefix is the root') t.same(config.get('workspace'), [], 'did not set workspace') - t.equal(logs.length, 0, 'got no log messages') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 0, 'got no log messages') }) t.test('does not error for invalid package.json', async (t) => { @@ -1354,8 +1362,9 @@ t.test('workspaces', async (t) => { await config.load() t.equal(config.localPrefix, path, 'localPrefix is the root') t.same(config.get('workspace'), [join(path, 'workspaces', 'one')], 'set the workspace') - t.equal(logs.length, 1, 'got one log message') - t.match(logs[0], ['info', /^found workspace root at/], 'logged info about workspace root') + const filtered = logs.filter(l => l[0] !== 'silly') + t.equal(filtered.length, 1, 'got one log message') + t.match(filtered[0], ['info', /^found workspace root at/], 'logged info about workspace root') }) }) @@ -1474,7 +1483,8 @@ t.test('catch project config prefix error', async t => { logs.length = 0 // config.load() triggers the error to be logged await config.load() - t.match(logs, [[ + const filtered = logs.filter(l => l[0] !== 'silly') + t.match(filtered, [[ 'error', 'config', `prefix cannot be changed from project config: ${path}`, ]], 'Expected error logged') }) From ee02b6ecad7499dbabf77b70c72bd34fa7349b1a Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 15:13:13 -0700 Subject: [PATCH 05/16] fix: remove some npm.load timers and exit earlier for --versions --- lib/cli/entry.js | 12 +- lib/npm.js | 103 ++++++--------- lib/utils/display.js | 32 ++++- lib/utils/log-file.js | 4 +- .../test/lib/cli/exit-handler.js.test.cjs | 66 ++++++++++ .../test/lib/utils/log-file.js.test.cjs | 119 +++++++++--------- test/lib/cli/entry.js | 16 ++- test/lib/{utils => cli}/exit-handler.js | 0 test/lib/npm.js | 16 +-- test/lib/utils/display.js | 9 +- 10 files changed, 208 insertions(+), 169 deletions(-) create mode 100644 tap-snapshots/test/lib/cli/exit-handler.js.test.cjs rename test/lib/{utils => cli}/exit-handler.js (100%) diff --git a/lib/cli/entry.js b/lib/cli/entry.js index 1c5f7e0dc1545..9b3e29ec336a6 100644 --- a/lib/cli/entry.js +++ b/lib/cli/entry.js @@ -40,20 +40,12 @@ module.exports = async (process, validateEngines) => { // Now actually fire up npm and run the command. // This is how to use npm programmatically: try { - await npm.load() + const { exec } = await npm.load() - // npm -v - if (npm.config.get('version', 'cli')) { - output.standard(npm.version) + if (!exec) { return exitHandler() } - // npm --versions - if (npm.config.get('versions', 'cli')) { - npm.argv = ['version'] - npm.config.set('usage', false, 'cli') - } - cmd = npm.argv.shift() if (!cmd) { output.standard(npm.usage) diff --git a/lib/npm.js b/lib/npm.js index d52f8bdc1c480..c801b33cf7adb 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -7,7 +7,7 @@ const usage = require('./utils/npm-usage.js') const LogFile = require('./utils/log-file.js') const Timers = require('./utils/timers.js') const Display = require('./utils/display.js') -const { log, time } = require('proc-log') +const { log, time, output } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const pkg = require('../package.json') const { deref } = require('./utils/cmd-list.js') @@ -28,20 +28,14 @@ class Npm { } updateNotification = null - loadErr = null argv = [] #command = null #runId = new Date().toISOString().replace(/[.:]/g, '_') - #loadPromise = null #title = 'npm' #argvClean = [] #npmRoot = null - #chalk = null - #logChalk = null - #noColorChalk = null - #display = null #logFile = new LogFile() #timers = new Timers({ start: 'npm' }) @@ -107,13 +101,7 @@ class Npm { } async load () { - if (!this.#loadPromise) { - this.#loadPromise = time.start('npm:load', () => this.#load().catch((er) => { - this.loadErr = er - throw er - })) - } - return this.#loadPromise + return time.start('npm:load', () => this.#load().then(r => r || { exec: true })) } get loaded () { @@ -160,19 +148,24 @@ class Npm { await time.start('npm:load:configload', () => this.config.load()) - // get createSupportsColor from chalk directly if this lands - // https://github.com/chalk/chalk/pull/600 - const [{ Chalk }, { createSupportsColor }] = await Promise.all([ - import('chalk'), - import('supports-color'), - ]) - this.#noColorChalk = new Chalk({ level: 0 }) - // we get the chalk level based on a null stream meaning chalk will only use - // what it knows about the environment to get color support since we already - // determined in our definitions that we want to show colors. - const level = Math.max(createSupportsColor(null).level, 1) - this.#chalk = this.color ? new Chalk({ level }) : this.#noColorChalk - this.#logChalk = this.logColor ? new Chalk({ level }) : this.#noColorChalk + await this.#display.load({ + loglevel: this.config.get('loglevel'), + stdoutColor: this.color, + stderrColor: this.logColor, + timing: this.config.get('timing'), + unicode: this.config.get('unicode'), + progress: this.flatOptions.progress, + json: this.config.get('json'), + heading: this.config.get('heading'), + }) + process.env.COLOR = this.color ? '1' : '0' + + // npm -v + // return from here early so we dont create any caches/logfiles/timers etc + if (this.config.get('version', 'cli')) { + output.standard(this.version) + return { exec: false } + } // mkdir this separately since the logs dir can be set to // a different location. if this fails, then we don't have @@ -208,49 +201,29 @@ class Npm { log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' ')) }) - time.start('npm:load:display', () => { - this.#display.load({ - loglevel: this.config.get('loglevel'), - // TODO: only pass in logColor and color and create chalk instances - // in display load method. Then remove chalk getters from npm and - // producers should emit chalk-templates (or something else). - stdoutChalk: this.#chalk, - stdoutColor: this.color, - stderrChalk: this.#logChalk, - stderrColor: this.logColor, - timing: this.config.get('timing'), - unicode: this.config.get('unicode'), - progress: this.flatOptions.progress, - json: this.config.get('json'), - heading: this.config.get('heading'), - }) - process.env.COLOR = this.color ? '1' : '0' + this.#logFile.load({ + path: this.logPath, + logsMax: this.config.get('logs-max'), }) - time.start('npm:load:logFile', () => { - this.#logFile.load({ - path: this.logPath, - logsMax: this.config.get('logs-max'), - }) - log.verbose('logfile', this.#logFile.files[0] || 'no logfile created') + this.#timers.load({ + path: this.config.get('timing') ? this.logPath : null, }) - time.start('npm:load:timers', () => - this.#timers.load({ - path: this.config.get('timing') ? this.logPath : null, - }) - ) - - time.start('npm:load:configScope', () => { - const configScope = this.config.get('scope') - if (configScope && !/^@/.test(configScope)) { - this.config.set('scope', `@${configScope}`, this.config.find('scope')) - } - }) + const configScope = this.config.get('scope') + if (configScope && !/^@/.test(configScope)) { + this.config.set('scope', `@${configScope}`, this.config.find('scope')) + } if (this.config.get('force')) { log.warn('using --force', 'Recommended protections disabled.') } + + // npm --versions + if (this.config.get('versions', 'cli')) { + this.argv = ['version'] + this.config.set('usage', false, 'cli') + } } get isShellout () { @@ -283,15 +256,15 @@ class Npm { } get noColorChalk () { - return this.#noColorChalk + return this.#display.chalk.noColor } get chalk () { - return this.#chalk + return this.#display.chalk.stdout } get logChalk () { - return this.#logChalk + return this.#display.chalk.stderr } get global () { diff --git a/lib/utils/display.js b/lib/utils/display.js index d9642fa374adf..1cc906083b5a9 100644 --- a/lib/utils/display.js +++ b/lib/utils/display.js @@ -121,6 +121,7 @@ class Display { } // colors + #noColorChalk #stdoutChalk #stdoutColor #stderrChalk @@ -161,25 +162,44 @@ class Display { } } - load ({ + get chalk () { + return { + noColor: this.#noColorChalk, + stdout: this.#stdoutChalk, + stderr: this.#stderrChalk, + } + } + + async load ({ heading, json, loglevel, progress, - stderrChalk, stderrColor, - stdoutChalk, stdoutColor, timing, unicode, }) { + // get createSupportsColor from chalk directly if this lands + // https://github.com/chalk/chalk/pull/600 + const [{ Chalk }, { createSupportsColor }] = await Promise.all([ + import('chalk'), + import('supports-color'), + ]) + // we get the chalk level based on a null stream meaning chalk will only use + // what it knows about the environment to get color support since we already + // determined in our definitions that we want to show colors. + const level = Math.max(createSupportsColor(null).level, 1) + + this.#noColorChalk = new Chalk({ level: 0 }) + this.#stdoutColor = stdoutColor - this.#stdoutChalk = stdoutChalk + this.#stdoutChalk = stdoutColor ? new Chalk({ level }) : this.#noColorChalk this.#stderrColor = stderrColor - this.#stderrChalk = stderrChalk + this.#stderrChalk = stderrColor ? new Chalk({ level }) : this.#noColorChalk - this.#logColors = COLOR_PALETTE({ chalk: stderrChalk }) + this.#logColors = COLOR_PALETTE({ chalk: this.#stderrChalk }) this.#levelIndex = LEVEL_OPTIONS[loglevel].index this.#timing = timing diff --git a/lib/utils/log-file.js b/lib/utils/log-file.js index a3792abf91951..80fd9d9636dbc 100644 --- a/lib/utils/log-file.js +++ b/lib/utils/log-file.js @@ -72,6 +72,8 @@ class LogFiles { } } + log.verbose('logfile', this.files[0] || 'no logfile created') + // Kickoff cleaning process, even if we aren't writing a logfile. // This is async but it will always ignore the current logfile // Return the result so it can be awaited in tests @@ -234,7 +236,7 @@ class LogFiles { } catch (e) { // Disable cleanup failure warnings when log writing is disabled if (this.#logsMax > 0) { - log.warn('logfile', 'error cleaning log files', e) + log.verbose('logfile', 'error cleaning log files', e) } } finally { log.silly('logfile', 'done cleaning log files') diff --git a/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs b/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs new file mode 100644 index 0000000000000..3a510647ece8e --- /dev/null +++ b/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs @@ -0,0 +1,66 @@ +/* IMPORTANT + * This snapshot file is auto-generated, but designed for humans. + * It should be checked into source control and tracked carefully. + * Re-generate by setting TAP_SNAPSHOT=1 and running tests. + * Make sure to inspect the output below. Do not ignore changes! + */ +'use strict' +exports[`test/lib/cli/exit-handler.js TAP handles unknown error with logs and debug file > debug file contents 1`] = ` +XX timing npm:load:whichnode Completed in {TIME}ms +XX silly config:load:file:{CWD}/npmrc +XX silly config:load:file:{CWD}/prefix/.npmrc +XX silly config:load:file:{CWD}/home/.npmrc +XX silly config:load:file:{CWD}/global/etc/npmrc +XX timing npm:load:configload Completed in {TIME}ms +XX timing npm:load:mkdirpcache Completed in {TIME}ms +XX timing npm:load:mkdirplogs Completed in {TIME}ms +XX verbose title npm +XX verbose argv "--fetch-retries" "0" "--cache" "{CWD}/cache" "--loglevel" "silly" "--color" "false" "--timing" "true" +XX timing npm:load:setTitle Completed in {TIME}ms +XX verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}- +XX verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log +XX timing npm:load Completed in {TIME}ms +XX verbose stack Error: Unknown error +XX verbose cwd {CWD}/prefix +XX verbose {OS} +XX verbose {NODE-VERSION} +XX verbose npm {NPM-VERSION} +XX error code ECODE +XX error ERR SUMMARY Unknown error +XX error ERR DETAIL Unknown error +XX verbose exit 1 +XX timing npm Completed in {TIME}ms +XX verbose code 1 +XX error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json +XX error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log +` + +exports[`test/lib/cli/exit-handler.js TAP handles unknown error with logs and debug file > logs 1`] = ` +timing npm:load:whichnode Completed in {TIME}ms +silly config:load:file:{CWD}/npmrc +silly config:load:file:{CWD}/prefix/.npmrc +silly config:load:file:{CWD}/home/.npmrc +silly config:load:file:{CWD}/global/etc/npmrc +timing npm:load:configload Completed in {TIME}ms +timing npm:load:mkdirpcache Completed in {TIME}ms +timing npm:load:mkdirplogs Completed in {TIME}ms +verbose title npm +verbose argv "--fetch-retries" "0" "--cache" "{CWD}/cache" "--loglevel" "silly" "--color" "false" "--timing" "true" +timing npm:load:setTitle Completed in {TIME}ms +verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}- +verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log +timing npm:load Completed in {TIME}ms +verbose stack Error: Unknown error +verbose cwd {CWD}/prefix +verbose {OS} +verbose {NODE-VERSION} +verbose npm {NPM-VERSION} +error code ECODE +error ERR SUMMARY Unknown error +error ERR DETAIL Unknown error +verbose exit 1 +timing npm Completed in {TIME}ms +verbose code 1 +error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json +error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log +` diff --git a/tap-snapshots/test/lib/utils/log-file.js.test.cjs b/tap-snapshots/test/lib/utils/log-file.js.test.cjs index 0a4af7cadf060..34002b8133e22 100644 --- a/tap-snapshots/test/lib/utils/log-file.js.test.cjs +++ b/tap-snapshots/test/lib/utils/log-file.js.test.cjs @@ -7,64 +7,65 @@ 'use strict' exports[`test/lib/utils/log-file.js TAP snapshot > must match snapshot 1`] = ` 0 verbose logfile logs-max:10 dir:{CWD}/{DATE}- -1 silly logfile done cleaning log files -2 error no prefix -3 error prefix with prefix -4 error prefix 1 2 3 -5 verbose { obj: { with: { many: [Object] } } } -6 verbose {"obj":{"with":{"many":{"props":1}}}} -7 verbose { -7 verbose "obj": { -7 verbose "with": { -7 verbose "many": { -7 verbose "props": 1 -7 verbose } -7 verbose } -7 verbose } -7 verbose } -8 verbose [ 'test', 'with', 'an', 'array' ] -9 verbose ["test","with","an","array"] -10 verbose [ -10 verbose "test", -10 verbose "with", -10 verbose "an", -10 verbose "array" -10 verbose ] -11 verbose [ 'test', [ 'with', [ 'an', [Array] ] ] ] -12 verbose ["test",["with",["an",["array"]]]] -13 verbose [ -13 verbose "test", -13 verbose [ -13 verbose "with", -13 verbose [ -13 verbose "an", -13 verbose [ -13 verbose "array" -13 verbose ] -13 verbose ] -13 verbose ] -13 verbose ] -14 error pre has many errors Error: message -14 error pre at stack trace line 0 -14 error pre at stack trace line 1 -14 error pre at stack trace line 2 -14 error pre at stack trace line 3 -14 error pre at stack trace line 4 -14 error pre at stack trace line 5 -14 error pre at stack trace line 6 -14 error pre at stack trace line 7 -14 error pre at stack trace line 8 -14 error pre at stack trace line 9 Error: message2 -14 error pre at stack trace line 0 -14 error pre at stack trace line 1 -14 error pre at stack trace line 2 -14 error pre at stack trace line 3 -14 error pre at stack trace line 4 -14 error pre at stack trace line 5 -14 error pre at stack trace line 6 -14 error pre at stack trace line 7 -14 error pre at stack trace line 8 -14 error pre at stack trace line 9 -15 error nostack [Error: message] +1 verbose logfile {CWD}/{DATE}-debug-0.log +2 silly logfile done cleaning log files +3 error no prefix +4 error prefix with prefix +5 error prefix 1 2 3 +6 verbose { obj: { with: { many: [Object] } } } +7 verbose {"obj":{"with":{"many":{"props":1}}}} +8 verbose { +8 verbose "obj": { +8 verbose "with": { +8 verbose "many": { +8 verbose "props": 1 +8 verbose } +8 verbose } +8 verbose } +8 verbose } +9 verbose [ 'test', 'with', 'an', 'array' ] +10 verbose ["test","with","an","array"] +11 verbose [ +11 verbose "test", +11 verbose "with", +11 verbose "an", +11 verbose "array" +11 verbose ] +12 verbose [ 'test', [ 'with', [ 'an', [Array] ] ] ] +13 verbose ["test",["with",["an",["array"]]]] +14 verbose [ +14 verbose "test", +14 verbose [ +14 verbose "with", +14 verbose [ +14 verbose "an", +14 verbose [ +14 verbose "array" +14 verbose ] +14 verbose ] +14 verbose ] +14 verbose ] +15 error pre has many errors Error: message +15 error pre at stack trace line 0 +15 error pre at stack trace line 1 +15 error pre at stack trace line 2 +15 error pre at stack trace line 3 +15 error pre at stack trace line 4 +15 error pre at stack trace line 5 +15 error pre at stack trace line 6 +15 error pre at stack trace line 7 +15 error pre at stack trace line 8 +15 error pre at stack trace line 9 Error: message2 +15 error pre at stack trace line 0 +15 error pre at stack trace line 1 +15 error pre at stack trace line 2 +15 error pre at stack trace line 3 +15 error pre at stack trace line 4 +15 error pre at stack trace line 5 +15 error pre at stack trace line 6 +15 error pre at stack trace line 7 +15 error pre at stack trace line 8 +15 error pre at stack trace line 9 +16 error nostack [Error: message] ` diff --git a/test/lib/cli/entry.js b/test/lib/cli/entry.js index 66a417e7d269a..feb6692704d2a 100644 --- a/test/lib/cli/entry.js +++ b/test/lib/cli/entry.js @@ -1,4 +1,5 @@ const t = require('tap') +const { dirname } = require('path') const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') const tmock = require('../../fixtures/tmock.js') const validateEngines = require('../../../lib/cli/validate-engines.js') @@ -29,20 +30,17 @@ const cliMock = async (t, opts) => { t.test('print the version, and treat npm_g as npm -g', async t => { const { logs, cli, Npm, outputs, exitHandlerCalled } = await cliMock(t, { - globals: { 'process.argv': ['node', 'npm_g', '-v'] }, + globals: { 'process.argv': ['node', 'npm_g', 'root'] }, }) await cli(process) - t.strictSame(process.argv, ['node', 'npm', '-g', '-v'], 'system process.argv was rewritten') + t.strictSame(process.argv, ['node', 'npm', '-g', 'root'], 'system process.argv was rewritten') t.strictSame(logs.verbose.byTitle('cli'), ['cli node npm']) - t.strictSame(logs.verbose.byTitle('title'), ['title npm']) - t.match(logs.verbose.byTitle('argv'), ['argv "--global" "--version"']) - t.strictSame(logs.info, [ - `using npm@${Npm.version}`, - `using node@${process.version}`, - ]) + t.strictSame(logs.verbose.byTitle('title'), ['title npm root']) + t.match(logs.verbose.byTitle('argv'), ['argv "--global" "root"']) + t.strictSame(logs.info, [`using npm@${Npm.version}`, `using node@${process.version}`]) t.equal(outputs.length, 1) - t.strictSame(outputs, [Npm.version]) + t.match(outputs[0], dirname(process.cwd())) t.strictSame(exitHandlerCalled(), []) }) diff --git a/test/lib/utils/exit-handler.js b/test/lib/cli/exit-handler.js similarity index 100% rename from test/lib/utils/exit-handler.js rename to test/lib/cli/exit-handler.js diff --git a/test/lib/npm.js b/test/lib/npm.js index bc9042365abea..915e63d0060bc 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -26,25 +26,13 @@ t.test('not yet loaded', async t => { t.test('npm.load', async t => { await t.test('load error', async t => { const { npm } = await loadMockNpm(t, { load: false }) - const loadError = new Error('load error') npm.config.load = async () => { - throw loadError + throw new Error('load error') } await t.rejects( () => npm.load(), /load error/ ) - - t.equal(npm.loadErr, loadError) - npm.config.load = async () => { - throw new Error('different error') - } - await t.rejects( - () => npm.load(), - /load error/, - 'loading again returns the original error' - ) - t.equal(npm.loadErr, loadError) }) await t.test('basic loading', async t => { @@ -416,7 +404,7 @@ t.test('timings', async t => { t.match(logs.timing.byTitle('npm'), [ /Completed in [0-9]+ms/, ]) - t.match(logs.silly, [ + t.match(logs.silly.byTitle('timing'), [ `timing Tried to end timer that doesn't exist: baz`, ]) t.notOk(npm.unfinishedTimers.has('foo'), 'foo timer is gone') diff --git a/test/lib/utils/display.js b/test/lib/utils/display.js index 31ea7bbed8add..b05690ebd8179 100644 --- a/test/lib/utils/display.js +++ b/test/lib/utils/display.js @@ -5,23 +5,22 @@ const mockGlobals = require('@npmcli/mock-globals') const { inspect } = require('util') const mockDisplay = async (t, { mocks, load } = {}) => { - const { Chalk } = await import('chalk') const { log, output } = require('proc-log') const logs = mockLogs() const Display = tmock(t, '{LIB}/utils/display', mocks) const display = new Display(logs.streams) - const displayLoad = (opts) => display.load({ + const displayLoad = async (opts) => display.load({ loglevel: 'silly', - stderrChalk: new Chalk({ level: 0 }), stderrColor: false, + stdoutColot: false, heading: 'npm', ...opts, }) if (load !== false) { - displayLoad(load) + await displayLoad(load) } t.teardown(() => display.off()) @@ -68,7 +67,7 @@ t.test('can buffer output when paused', async t => { output.standard('Message 2') t.strictSame(outputs, []) - displayLoad() + await displayLoad() t.strictSame(outputs, ['Message 1', 'Message 2']) }) From 2994ee877f0d360bd7cffda3d6db38dd07af1d5e Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 15:36:18 -0700 Subject: [PATCH 06/16] chore: move sigstore json files to fixtures dir --- .../sigstore/valid-sigstore-attestations.json | 0 .../sigstore/valid-tuf-js-attestations.json | 0 test/lib/commands/audit.js | 14 +++++++------- 3 files changed, 7 insertions(+), 7 deletions(-) rename test/{lib => }/fixtures/sigstore/valid-sigstore-attestations.json (100%) rename test/{lib => }/fixtures/sigstore/valid-tuf-js-attestations.json (100%) diff --git a/test/lib/fixtures/sigstore/valid-sigstore-attestations.json b/test/fixtures/sigstore/valid-sigstore-attestations.json similarity index 100% rename from test/lib/fixtures/sigstore/valid-sigstore-attestations.json rename to test/fixtures/sigstore/valid-sigstore-attestations.json diff --git a/test/lib/fixtures/sigstore/valid-tuf-js-attestations.json b/test/fixtures/sigstore/valid-tuf-js-attestations.json similarity index 100% rename from test/lib/fixtures/sigstore/valid-tuf-js-attestations.json rename to test/fixtures/sigstore/valid-tuf-js-attestations.json diff --git a/test/lib/commands/audit.js b/test/lib/commands/audit.js index 51abae13322cc..701d374ade985 100644 --- a/test/lib/commands/audit.js +++ b/test/lib/commands/audit.js @@ -1892,7 +1892,7 @@ t.test('audit signatures', async t => { const registry = new MockRegistry({ tap: t, registry: npm.config.get('registry') }) await manifestWithValidAttestations({ registry }) const fixture = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-sigstore-attestations.json'), + path.resolve(__dirname, '../../fixtures/sigstore/valid-sigstore-attestations.json'), 'utf8' ) registry.nock.get('/-/npm/v1/attestations/sigstore@1.0.0').reply(200, fixture) @@ -1918,11 +1918,11 @@ t.test('audit signatures', async t => { await manifestWithValidAttestations({ registry }) await manifestWithMultipleValidAttestations({ registry }) const fixture1 = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-sigstore-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-sigstore-attestations.json'), 'utf8' ) const fixture2 = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-tuf-js-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-tuf-js-attestations.json'), 'utf8' ) registry.nock.get('/-/npm/v1/attestations/sigstore@1.0.0').reply(200, fixture1) @@ -1951,7 +1951,7 @@ t.test('audit signatures', async t => { const registry = new MockRegistry({ tap: t, registry: npm.config.get('registry') }) await manifestWithValidAttestations({ registry }) const fixture = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-sigstore-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-sigstore-attestations.json'), 'utf8' ) registry.nock.get('/-/npm/v1/attestations/sigstore@1.0.0').reply(200, fixture) @@ -1986,7 +1986,7 @@ t.test('audit signatures', async t => { const registry = new MockRegistry({ tap: t, registry: npm.config.get('registry') }) await manifestWithValidAttestations({ registry }) const fixture = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-sigstore-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-sigstore-attestations.json'), 'utf8' ) registry.nock.get('/-/npm/v1/attestations/sigstore@1.0.0').reply(200, fixture) @@ -2016,11 +2016,11 @@ t.test('audit signatures', async t => { await manifestWithValidAttestations({ registry }) await manifestWithMultipleValidAttestations({ registry }) const fixture1 = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-sigstore-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-sigstore-attestations.json'), 'utf8' ) const fixture2 = fs.readFileSync( - path.join(__dirname, '..', 'fixtures', 'sigstore/valid-tuf-js-attestations.json'), + path.join(__dirname, '../../fixtures/sigstore/valid-tuf-js-attestations.json'), 'utf8' ) registry.nock.get('/-/npm/v1/attestations/sigstore@1.0.0').reply(200, fixture1) From f9541c8aa150e1103fdc79d65c331ab5c242aa99 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 15:39:50 -0700 Subject: [PATCH 07/16] fix: rename base-cmd to match other commands --- lib/arborist-cmd.js | 3 +-- lib/{base-command.js => base-cmd.js} | 0 lib/commands/access.js | 2 +- lib/commands/adduser.js | 2 +- lib/commands/cache.js | 2 +- lib/commands/completion.js | 2 +- lib/commands/config.js | 2 +- lib/commands/deprecate.js | 2 +- lib/commands/diff.js | 2 +- lib/commands/dist-tag.js | 2 +- lib/commands/doctor.js | 2 +- lib/commands/edit.js | 2 +- lib/commands/exec.js | 2 +- lib/commands/explore.js | 2 +- lib/commands/get.js | 2 +- lib/commands/help-search.js | 2 +- lib/commands/help.js | 2 +- lib/commands/hook.js | 2 +- lib/commands/init.js | 2 +- lib/commands/login.js | 2 +- lib/commands/logout.js | 2 +- lib/commands/org.js | 2 +- lib/commands/owner.js | 2 +- lib/commands/pack.js | 2 +- lib/commands/ping.js | 2 +- lib/commands/pkg.js | 2 +- lib/commands/prefix.js | 2 +- lib/commands/profile.js | 2 +- lib/commands/publish.js | 2 +- lib/commands/query.js | 2 +- lib/commands/root.js | 2 +- lib/commands/run-script.js | 2 +- lib/commands/sbom.js | 2 +- lib/commands/search.js | 2 +- lib/commands/set.js | 2 +- lib/commands/shrinkwrap.js | 2 +- lib/commands/star.js | 2 +- lib/commands/stars.js | 2 +- lib/commands/team.js | 2 +- lib/commands/token.js | 2 +- lib/commands/unpublish.js | 2 +- lib/commands/version.js | 2 +- lib/commands/view.js | 2 +- lib/commands/whoami.js | 2 +- lib/lifecycle-cmd.js | 4 ++-- lib/package-url-cmd.js | 6 ++---- test/lib/load-all-commands.js | 2 +- 47 files changed, 48 insertions(+), 51 deletions(-) rename lib/{base-command.js => base-cmd.js} (100%) diff --git a/lib/arborist-cmd.js b/lib/arborist-cmd.js index d8cb25baf487e..9d247d02fa181 100644 --- a/lib/arborist-cmd.js +++ b/lib/arborist-cmd.js @@ -1,10 +1,9 @@ const { log } = require('proc-log') +const BaseCommand = require('./base-cmd.js') // This is the base for all commands whose execWorkspaces just gets // a list of workspace names and passes it on to new Arborist() to // be able to run a filtered Arborist.reify() at some point. - -const BaseCommand = require('./base-command.js') class ArboristCmd extends BaseCommand { get isArboristCmd () { return true diff --git a/lib/base-command.js b/lib/base-cmd.js similarity index 100% rename from lib/base-command.js rename to lib/base-cmd.js diff --git a/lib/commands/access.js b/lib/commands/access.js index 1d0161cadc91e..d35699e839109 100644 --- a/lib/commands/access.js +++ b/lib/commands/access.js @@ -5,7 +5,7 @@ const pkgJson = require('@npmcli/package-json') const localeCompare = require('@isaacs/string-locale-compare')('en') const otplease = require('../utils/otplease.js') const getIdentity = require('../utils/get-identity.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const commands = [ 'get', diff --git a/lib/commands/adduser.js b/lib/commands/adduser.js index d896272bf7ed5..5299ebaca41aa 100644 --- a/lib/commands/adduser.js +++ b/lib/commands/adduser.js @@ -1,7 +1,7 @@ const { log, output } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const auth = require('../utils/auth.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class AddUser extends BaseCommand { static description = 'Add a registry user account' diff --git a/lib/commands/cache.js b/lib/commands/cache.js index 8e9b33b3d1a49..108c261ffe57d 100644 --- a/lib/commands/cache.js +++ b/lib/commands/cache.js @@ -3,7 +3,7 @@ const pacote = require('pacote') const fs = require('fs/promises') const { join } = require('path') const semver = require('semver') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const npa = require('npm-package-arg') const jsonParse = require('json-parse-even-better-errors') const localeCompare = require('@isaacs/string-locale-compare')('en') diff --git a/lib/commands/completion.js b/lib/commands/completion.js index 20ff3ea76e98b..a4cf1d77c5e3a 100644 --- a/lib/commands/completion.js +++ b/lib/commands/completion.js @@ -36,7 +36,7 @@ const Npm = require('../npm.js') const { definitions, shorthands } = require('@npmcli/config/lib/definitions') const { commands, aliases, deref } = require('../utils/cmd-list.js') const { isWindowsShell } = require('../utils/is-windows.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const fileExists = (file) => fs.stat(file).then(s => s.isFile()).catch(() => false) diff --git a/lib/commands/config.js b/lib/commands/config.js index 56315208e232f..7fb8476276937 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -6,7 +6,7 @@ const localeCompare = require('@isaacs/string-locale-compare')('en') const pkgJson = require('@npmcli/package-json') const { defaults, definitions } = require('@npmcli/config/lib/definitions') const { log, output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') // These are the configs that we can nerf-dart. Not all of them currently even // *have* config definitions so we have to explicitly validate them here diff --git a/lib/commands/deprecate.js b/lib/commands/deprecate.js index bdce313923cff..58856538fe23f 100644 --- a/lib/commands/deprecate.js +++ b/lib/commands/deprecate.js @@ -4,7 +4,7 @@ const npa = require('npm-package-arg') const semver = require('semver') const getIdentity = require('../utils/get-identity.js') const libaccess = require('libnpmaccess') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Deprecate extends BaseCommand { static description = 'Deprecate a version of a package' diff --git a/lib/commands/diff.js b/lib/commands/diff.js index e188a38505867..6b5adf633acd7 100644 --- a/lib/commands/diff.js +++ b/lib/commands/diff.js @@ -6,7 +6,7 @@ const pacote = require('pacote') const pickManifest = require('npm-pick-manifest') const { log, output } = require('proc-log') const pkgJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Diff extends BaseCommand { static description = 'The registry diff command' diff --git a/lib/commands/dist-tag.js b/lib/commands/dist-tag.js index 7eeeae571bc1c..e13f9ecf59c7f 100644 --- a/lib/commands/dist-tag.js +++ b/lib/commands/dist-tag.js @@ -4,7 +4,7 @@ const semver = require('semver') const { log, output } = require('proc-log') const otplease = require('../utils/otplease.js') const pkgJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class DistTag extends BaseCommand { static description = 'Modify package distribution tags' diff --git a/lib/commands/doctor.js b/lib/commands/doctor.js index a9f24bd62e0f0..339dcf15f70f2 100644 --- a/lib/commands/doctor.js +++ b/lib/commands/doctor.js @@ -8,7 +8,7 @@ const semver = require('semver') const { log, output } = require('proc-log') const ping = require('../utils/ping.js') const { defaults } = require('@npmcli/config/lib/definitions') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const maskLabel = mask => { const label = [] diff --git a/lib/commands/edit.js b/lib/commands/edit.js index 12c9f759589e7..4110a1db55e82 100644 --- a/lib/commands/edit.js +++ b/lib/commands/edit.js @@ -2,7 +2,7 @@ const { resolve } = require('path') const { lstat } = require('fs/promises') const cp = require('child_process') const completion = require('../utils/installed-shallow.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const splitPackageNames = (path) => path.split('/') // combine scoped parts diff --git a/lib/commands/exec.js b/lib/commands/exec.js index f181bbb2ef6fe..9bb4b15e0c5a3 100644 --- a/lib/commands/exec.js +++ b/lib/commands/exec.js @@ -1,6 +1,6 @@ const { resolve } = require('path') const libexec = require('libnpmexec') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Exec extends BaseCommand { static description = 'Run a command from a local or remote npm package' diff --git a/lib/commands/explore.js b/lib/commands/explore.js index a4acc821eabf9..d9dd9a9931f56 100644 --- a/lib/commands/explore.js +++ b/lib/commands/explore.js @@ -3,7 +3,7 @@ const runScript = require('@npmcli/run-script') const { join, relative } = require('path') const { log, output } = require('proc-log') const completion = require('../utils/installed-shallow.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') // npm explore [@] // open a subshell to the package folder. diff --git a/lib/commands/get.js b/lib/commands/get.js index 43eca7389ec90..4191f2c973e7d 100644 --- a/lib/commands/get.js +++ b/lib/commands/get.js @@ -1,5 +1,5 @@ const Npm = require('../npm.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Get extends BaseCommand { static description = 'Get a value from the npm configuration' diff --git a/lib/commands/help-search.js b/lib/commands/help-search.js index 1bbff2df6cc16..67d381dd94565 100644 --- a/lib/commands/help-search.js +++ b/lib/commands/help-search.js @@ -2,7 +2,7 @@ const { readFile } = require('node:fs/promises') const path = require('node:path') const { glob } = require('glob') const { output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const globify = pattern => pattern.split('\\').join('/') diff --git a/lib/commands/help.js b/lib/commands/help.js index fd84d3f8546ef..fb3fe664e017d 100644 --- a/lib/commands/help.js +++ b/lib/commands/help.js @@ -5,7 +5,7 @@ const { glob } = require('glob') const { output } = require('proc-log') const localeCompare = require('@isaacs/string-locale-compare')('en') const { deref } = require('../utils/cmd-list.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const globify = pattern => pattern.split('\\').join('/') diff --git a/lib/commands/hook.js b/lib/commands/hook.js index 557d04e7a035a..3b91ff539081a 100644 --- a/lib/commands/hook.js +++ b/lib/commands/hook.js @@ -2,7 +2,7 @@ const hookApi = require('libnpmhook') const otplease = require('../utils/otplease.js') const relativeDate = require('tiny-relative-date') const { output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Hook extends BaseCommand { static description = 'Manage registry hooks' diff --git a/lib/commands/init.js b/lib/commands/init.js index cee56e7b91ba5..1847e19a9560f 100644 --- a/lib/commands/init.js +++ b/lib/commands/init.js @@ -8,7 +8,7 @@ const mapWorkspaces = require('@npmcli/map-workspaces') const PackageJson = require('@npmcli/package-json') const { log, output } = require('proc-log') const updateWorkspaces = require('../utils/update-workspaces.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const posixPath = p => p.split('\\').join('/') diff --git a/lib/commands/login.js b/lib/commands/login.js index 0801af9726f78..fa82ecaa2c207 100644 --- a/lib/commands/login.js +++ b/lib/commands/login.js @@ -1,7 +1,7 @@ const { log, output } = require('proc-log') const { redactLog: replaceInfo } = require('@npmcli/redact') const auth = require('../utils/auth.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Login extends BaseCommand { static description = 'Login to a registry user account' diff --git a/lib/commands/logout.js b/lib/commands/logout.js index 63a8154cd3fdf..39b4b176bf966 100644 --- a/lib/commands/logout.js +++ b/lib/commands/logout.js @@ -1,7 +1,7 @@ const npmFetch = require('npm-registry-fetch') const { getAuth } = npmFetch const { log } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Logout extends BaseCommand { static description = 'Log out of the registry' diff --git a/lib/commands/org.js b/lib/commands/org.js index f06c537d3f3f7..af67547a643db 100644 --- a/lib/commands/org.js +++ b/lib/commands/org.js @@ -1,6 +1,6 @@ const liborg = require('libnpmorg') const otplease = require('../utils/otplease.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const { output } = require('proc-log') class Org extends BaseCommand { diff --git a/lib/commands/owner.js b/lib/commands/owner.js index cfd40df2151e6..188065583198d 100644 --- a/lib/commands/owner.js +++ b/lib/commands/owner.js @@ -4,7 +4,7 @@ const pacote = require('pacote') const { log, output } = require('proc-log') const otplease = require('../utils/otplease.js') const pkgJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const { redact } = require('@npmcli/redact') const readJson = async (path) => { diff --git a/lib/commands/pack.js b/lib/commands/pack.js index e679377fa7970..f64a21dcc0d9d 100644 --- a/lib/commands/pack.js +++ b/lib/commands/pack.js @@ -3,7 +3,7 @@ const libpack = require('libnpmpack') const npa = require('npm-package-arg') const { log, output } = require('proc-log') const { getContents, logTar } = require('../utils/tar.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Pack extends BaseCommand { static description = 'Create a tarball from a package' diff --git a/lib/commands/ping.js b/lib/commands/ping.js index 2d8f6f91a9111..8ade46ac8f82a 100644 --- a/lib/commands/ping.js +++ b/lib/commands/ping.js @@ -1,7 +1,7 @@ const { redact } = require('@npmcli/redact') const { log, output } = require('proc-log') const pingUtil = require('../utils/ping.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Ping extends BaseCommand { static description = 'Ping npm registry' diff --git a/lib/commands/pkg.js b/lib/commands/pkg.js index 26e60fec48786..62553b15103e3 100644 --- a/lib/commands/pkg.js +++ b/lib/commands/pkg.js @@ -1,6 +1,6 @@ const { output } = require('proc-log') const PackageJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const Queryable = require('../utils/queryable.js') class Pkg extends BaseCommand { diff --git a/lib/commands/prefix.js b/lib/commands/prefix.js index edd5595d09b41..d1f56cb190bd7 100644 --- a/lib/commands/prefix.js +++ b/lib/commands/prefix.js @@ -1,5 +1,5 @@ const { output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Prefix extends BaseCommand { static description = 'Display prefix' diff --git a/lib/commands/profile.js b/lib/commands/profile.js index a0676e47889af..66bb3d118e6b7 100644 --- a/lib/commands/profile.js +++ b/lib/commands/profile.js @@ -5,7 +5,7 @@ const npmProfile = require('npm-profile') const qrcodeTerminal = require('qrcode-terminal') const otplease = require('../utils/otplease.js') const readUserInfo = require('../utils/read-user-info.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const qrcode = url => new Promise((resolve) => qrcodeTerminal.generate(url, resolve)) diff --git a/lib/commands/publish.js b/lib/commands/publish.js index f174d338b6392..fba7b7a120953 100644 --- a/lib/commands/publish.js +++ b/lib/commands/publish.js @@ -15,7 +15,7 @@ const { getContents, logTar } = require('../utils/tar.js') // change that ought to get a RFC written on it. const { flatten } = require('@npmcli/config/lib/definitions') const pkgJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Publish extends BaseCommand { static description = 'Publish a package' diff --git a/lib/commands/query.js b/lib/commands/query.js index 1e9dee4ef1fda..fe84469b88fe0 100644 --- a/lib/commands/query.js +++ b/lib/commands/query.js @@ -1,5 +1,5 @@ const { resolve } = require('node:path') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const { log, output } = require('proc-log') class QuerySelectorItem { diff --git a/lib/commands/root.js b/lib/commands/root.js index f7c330a004169..180f4c4ed0720 100644 --- a/lib/commands/root.js +++ b/lib/commands/root.js @@ -1,5 +1,5 @@ const { output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Root extends BaseCommand { static description = 'Display npm root' diff --git a/lib/commands/run-script.js b/lib/commands/run-script.js index f048996e66354..0bf517f77efb4 100644 --- a/lib/commands/run-script.js +++ b/lib/commands/run-script.js @@ -1,6 +1,6 @@ const { log, output } = require('proc-log') const pkgJson = require('@npmcli/package-json') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class RunScript extends BaseCommand { static description = 'Run arbitrary package scripts' diff --git a/lib/commands/sbom.js b/lib/commands/sbom.js index 4ee21ec1954fd..ff7377581dfa5 100644 --- a/lib/commands/sbom.js +++ b/lib/commands/sbom.js @@ -1,5 +1,5 @@ const localeCompare = require('@isaacs/string-locale-compare')('en') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const { log, output } = require('proc-log') const { cyclonedxOutput } = require('../utils/sbom-cyclonedx.js') const { spdxOutput } = require('../utils/sbom-spdx.js') diff --git a/lib/commands/search.js b/lib/commands/search.js index cffc4cdacc846..93005398480d1 100644 --- a/lib/commands/search.js +++ b/lib/commands/search.js @@ -2,8 +2,8 @@ const Pipeline = require('minipass-pipeline') const libSearch = require('libnpmsearch') const { log, output } = require('proc-log') const formatSearchStream = require('../utils/format-search-stream.js') +const BaseCommand = require('../base-cmd.js') -const BaseCommand = require('../base-command.js') class Search extends BaseCommand { static description = 'Search for packages' static name = 'search' diff --git a/lib/commands/set.js b/lib/commands/set.js index 509da1956bf1f..2e61762ba9dcd 100644 --- a/lib/commands/set.js +++ b/lib/commands/set.js @@ -1,5 +1,5 @@ const Npm = require('../npm.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Set extends BaseCommand { static description = 'Set a value in the npm configuration' diff --git a/lib/commands/shrinkwrap.js b/lib/commands/shrinkwrap.js index 008244e9760f3..d7866bdb91dce 100644 --- a/lib/commands/shrinkwrap.js +++ b/lib/commands/shrinkwrap.js @@ -1,7 +1,7 @@ const { resolve, basename } = require('path') const { unlink } = require('fs/promises') const { log } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Shrinkwrap extends BaseCommand { static description = 'Lock down dependency versions for publication' diff --git a/lib/commands/star.js b/lib/commands/star.js index fbf81e64bf21d..1b76955810c72 100644 --- a/lib/commands/star.js +++ b/lib/commands/star.js @@ -2,7 +2,7 @@ const fetch = require('npm-registry-fetch') const npa = require('npm-package-arg') const { log, output } = require('proc-log') const getIdentity = require('../utils/get-identity') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Star extends BaseCommand { static description = 'Mark your favorite packages' diff --git a/lib/commands/stars.js b/lib/commands/stars.js index 628b22fdb8808..1059569979daf 100644 --- a/lib/commands/stars.js +++ b/lib/commands/stars.js @@ -1,7 +1,7 @@ const fetch = require('npm-registry-fetch') const { log, output } = require('proc-log') const getIdentity = require('../utils/get-identity.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Stars extends BaseCommand { static description = 'View packages marked as favorites' diff --git a/lib/commands/team.js b/lib/commands/team.js index f7bada28a611e..22af61863851a 100644 --- a/lib/commands/team.js +++ b/lib/commands/team.js @@ -3,7 +3,7 @@ const libteam = require('libnpmteam') const { output } = require('proc-log') const otplease = require('../utils/otplease.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Team extends BaseCommand { static description = 'Manage organization teams and team memberships' static name = 'team' diff --git a/lib/commands/token.js b/lib/commands/token.js index 4429a20319f7e..aa9bd0fce24f8 100644 --- a/lib/commands/token.js +++ b/lib/commands/token.js @@ -2,7 +2,7 @@ const { log, output } = require('proc-log') const profile = require('npm-profile') const otplease = require('../utils/otplease.js') const readUserInfo = require('../utils/read-user-info.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Token extends BaseCommand { static description = 'Manage your authentication tokens' diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index e8b7de8395ea9..47a5db8206244 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -7,7 +7,7 @@ const pkgJson = require('@npmcli/package-json') const { flatten } = require('@npmcli/config/lib/definitions') const getIdentity = require('../utils/get-identity.js') const otplease = require('../utils/otplease.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const LAST_REMAINING_VERSION_ERROR = 'Refusing to delete the last version of the package. ' + 'It will block from republishing a new version for 24 hours.\n' + diff --git a/lib/commands/version.js b/lib/commands/version.js index 4d7b971fa6fbf..549ba9b9f9c77 100644 --- a/lib/commands/version.js +++ b/lib/commands/version.js @@ -1,7 +1,7 @@ const { resolve } = require('node:path') const { readFile } = require('node:fs/promises') const { output } = require('proc-log') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Version extends BaseCommand { static description = 'Bump a package version' diff --git a/lib/commands/view.js b/lib/commands/view.js index 9c8630bef154e..c0d5bf552eee0 100644 --- a/lib/commands/view.js +++ b/lib/commands/view.js @@ -10,7 +10,7 @@ const semver = require('semver') const { inspect } = require('util') const { packument } = require('pacote') const Queryable = require('../utils/queryable.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') const readJson = async file => jsonParse(await readFile(file, 'utf8')) diff --git a/lib/commands/whoami.js b/lib/commands/whoami.js index 03d7bb24991cf..df749e9db7e91 100644 --- a/lib/commands/whoami.js +++ b/lib/commands/whoami.js @@ -1,6 +1,6 @@ const { output } = require('proc-log') const getIdentity = require('../utils/get-identity.js') -const BaseCommand = require('../base-command.js') +const BaseCommand = require('../base-cmd.js') class Whoami extends BaseCommand { static description = 'Display npm username' diff --git a/lib/lifecycle-cmd.js b/lib/lifecycle-cmd.js index 26e04b12af272..a509a9380f668 100644 --- a/lib/lifecycle-cmd.js +++ b/lib/lifecycle-cmd.js @@ -1,7 +1,7 @@ +const BaseCommand = require('./base-cmd.js') + // The implementation of commands that are just "run a script" // restart, start, stop, test - -const BaseCommand = require('./base-command.js') class LifecycleCmd extends BaseCommand { static usage = ['[-- ]'] static isShellout = true diff --git a/lib/package-url-cmd.js b/lib/package-url-cmd.js index 5ab7cb87a2cd3..bcefd17af4492 100644 --- a/lib/package-url-cmd.js +++ b/lib/package-url-cmd.js @@ -1,11 +1,9 @@ -// Base command for opening urls from a package manifest (bugs, docs, repo) - const pacote = require('pacote') - const openUrl = require('./utils/open-url.js') const { log } = require('proc-log') +const BaseCommand = require('./base-cmd.js') -const BaseCommand = require('./base-command.js') +// Base command for opening urls from a package manifest (bugs, docs, repo) class PackageUrlCommand extends BaseCommand { static params = [ 'browser', diff --git a/test/lib/load-all-commands.js b/test/lib/load-all-commands.js index 9fa6995cfb1ad..c00da37eff3fc 100644 --- a/test/lib/load-all-commands.js +++ b/test/lib/load-all-commands.js @@ -6,7 +6,7 @@ const t = require('tap') const util = require('util') const { load: loadMockNpm } = require('../fixtures/mock-npm.js') const { commands } = require('../../lib/utils/cmd-list.js') -const BaseCommand = require('../../lib/base-command.js') +const BaseCommand = require('../../lib/base-cmd.js') const isAsyncFn = (v) => typeof v === 'function' && /^\[AsyncFunction:/.test(util.inspect(v)) From 47554637f64fd131114262a2ec75bb54f9c3f696 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 16:04:58 -0700 Subject: [PATCH 08/16] fix: dont write timing logs to file unless requested --- lib/npm.js | 1 + lib/utils/log-file.js | 42 +++++++++++++++++++++++--------------- test/lib/utils/log-file.js | 2 ++ 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/lib/npm.js b/lib/npm.js index c801b33cf7adb..fcccdbf7381a0 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -204,6 +204,7 @@ class Npm { this.#logFile.load({ path: this.logPath, logsMax: this.config.get('logs-max'), + timing: this.config.get('timing'), }) this.#timers.load({ diff --git a/lib/utils/log-file.js b/lib/utils/log-file.js index 80fd9d9636dbc..09e3873f2dce6 100644 --- a/lib/utils/log-file.js +++ b/lib/utils/log-file.js @@ -1,6 +1,5 @@ const os = require('os') const { join, dirname, basename } = require('path') -const { Minipass } = require('minipass') const fsMiniPass = require('fs-minipass') const fs = require('fs/promises') const { log } = require('proc-log') @@ -9,9 +8,9 @@ const { formatWithOptions } = require('./format') const padZero = (n, length) => n.toString().padStart(length.toString().length, '0') class LogFiles { - // Default to a plain minipass stream so we can buffer + // Default to an array so we can buffer // initial writes before we know the cache location - #logStream = null + #logStream = [] // We cap log files at a certain number of log events per file. // Note that each log event can write more than one line to the @@ -29,6 +28,7 @@ class LogFiles { #path = null #logsMax = null #files = [] + #timing = false constructor ({ maxLogsPerFile = 50_000, @@ -40,7 +40,6 @@ class LogFiles { } on () { - this.#logStream = new Minipass() process.on('log', this.#logHandler) } @@ -49,11 +48,12 @@ class LogFiles { this.#endStream() } - load ({ path, logsMax = Infinity } = {}) { + load ({ path, logsMax = Infinity, timing } = {}) { // dir is user configurable and is required to exist so // this can error if the dir is missing or not configured correctly this.#path = path this.#logsMax = logsMax + this.#timing = timing // Log stream has already ended if (!this.#logStream) { @@ -62,13 +62,19 @@ class LogFiles { log.verbose('logfile', `logs-max:${logsMax} dir:${this.#path}`) - // Pipe our initial stream to our new file stream and + // Write the contents of our array buffer to our new file stream and // set that as the new log logstream for future writes // if logs max is 0 then the user does not want a log file if (this.#logsMax > 0) { const initialFile = this.#openLogFile() if (initialFile) { - this.#logStream = this.#logStream.pipe(initialFile) + for (const item of this.#logStream) { + const formatted = this.#formatLogItem(...item) + if (formatted !== null) { + initialFile.write(formatted) + } + } + this.#logStream = initialFile } } @@ -80,20 +86,16 @@ class LogFiles { return this.#cleanLogs() } - log (...args) { - this.#logHandler(...args) - } - get files () { return this.#files } get #isBuffered () { - return this.#logStream instanceof Minipass + return Array.isArray(this.#logStream) } #endStream (output) { - if (this.#logStream) { + if (this.#logStream && !this.#isBuffered) { this.#logStream.end(output) this.#logStream = null } @@ -111,12 +113,15 @@ class LogFiles { return } - const logOutput = this.#formatLogItem(level, ...args) - if (this.#isBuffered) { // Cant do anything but buffer the output if we dont // have a file stream yet - this.#logStream.write(logOutput) + this.#logStream.push([level, ...args]) + return + } + + const logOutput = this.#formatLogItem(level, ...args) + if (logOutput === null) { return } @@ -137,6 +142,11 @@ class LogFiles { } #formatLogItem (level, title, ...args) { + // Only right timing logs to logfile if explicitly requests + if (level === log.KEYS.timing && !this.#timing) { + return null + } + this.#fileLogCount += 1 const prefix = [this.#totalLogCount++, level, title || null] return formatWithOptions({ prefix, eol: os.EOL, colors: false }, ...args) diff --git a/test/lib/utils/log-file.js b/test/lib/utils/log-file.js index f34dda8f52433..a4cf8a256a634 100644 --- a/test/lib/utils/log-file.js +++ b/test/lib/utils/log-file.js @@ -46,6 +46,8 @@ const loadLogFile = async (t, { buffer = [], mocks, testdir = {}, ...options } = const MockLogFile = tmock(t, '{LIB}/utils/log-file.js', mocks) const logFile = new MockLogFile(Object.keys(options).length ? options : undefined) + // Create a fake public method since there is not one on logFile anymore + logFile.log = (...b) => process.emit('log', ...b) buffer.forEach((b) => logFile.log(...b)) const id = getId() From 57e945bd41f3039bf65eca2a15ecf235547bd00e Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 16:39:27 -0700 Subject: [PATCH 09/16] fix: use proc-log META for flush and force --- lib/cli/exit-handler.js | 15 ++--- lib/npm.js | 10 --- lib/utils/display.js | 122 ++++++++++++++--------------------- lib/utils/error-message.js | 8 +-- test/fixtures/mock-npm.js | 4 +- test/lib/cli/exit-handler.js | 8 +-- 6 files changed, 64 insertions(+), 103 deletions(-) diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js index 6dc44de1f003a..8d1d5a3e0ef34 100644 --- a/lib/cli/exit-handler.js +++ b/lib/cli/exit-handler.js @@ -1,4 +1,4 @@ -const { log, output, time } = require('proc-log') +const { log, output, time, META } = require('proc-log') const errorMessage = require('../utils/error-message.js') const { redactLog: replaceInfo } = require('@npmcli/redact') @@ -32,9 +32,10 @@ process.on('exit', code => { log.error('', 'This is an error with npm itself. Please report this error at:') log.error('', ' ') - // This gets logged regardless of silent mode - // eslint-disable-next-line no-console - console.error('') + if (!npm || !npm.silent) { + // eslint-disable-next-line no-console + console.error('') + } showLogFileError = true } @@ -120,8 +121,7 @@ const exitHandler = err => { // only show the notification if it finished. if (typeof npm.updateNotification === 'string') { - // TODO: use proc-log to send `force: true` along with event - npm.forceLog('notice', '', npm.updateNotification) + log.notice('', npm.updateNotification, { [META]: true, force: true }) } let exitCode = process.exitCode || 0 @@ -200,8 +200,7 @@ const exitHandler = err => { } if (hasLoadedNpm) { - // TODO: use proc-log.output.flush() once it exists - npm.flushOutput(jsonError) + output.flush({ [META]: true, jsonError }) } log.verbose('exit', exitCode || 0) diff --git a/lib/npm.js b/lib/npm.js index fcccdbf7381a0..e0f3f40a72874 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -379,16 +379,6 @@ class Npm { get usage () { return usage(this) } - - // TODO: move to proc-log and remove - forceLog (...args) { - this.#display.forceLog(...args) - } - - // TODO: move to proc-log and remove - flushOutput (jsonError) { - this.#display.flushOutput(jsonError) - } } module.exports = Npm diff --git a/lib/utils/display.js b/lib/utils/display.js index 1cc906083b5a9..32399a7bf5329 100644 --- a/lib/utils/display.js +++ b/lib/utils/display.js @@ -1,5 +1,5 @@ const proggy = require('proggy') -const { log, output } = require('proc-log') +const { log, output, META } = require('proc-log') const { explain } = require('./explain-eresolve.js') const { formatWithOptions } = require('./format') @@ -32,17 +32,6 @@ const COLOR_PALETTE = ({ chalk: c }) => ({ silly: c.blue.dim, }) -const LOG_LEVELS = log.LEVELS.reduce((acc, key) => { - acc[key] = key - return acc -}, {}) - -// TODO: move flush to proc-log -const OUTPUT_LEVELS = ['flush', ...output.LEVELS].reduce((acc, key) => { - acc[key] = key - return acc -}, {}) - const LEVEL_OPTIONS = { silent: { index: 0, @@ -76,7 +65,7 @@ const LEVEL_OPTIONS = { const LEVEL_METHODS = { ...LEVEL_OPTIONS, - [LOG_LEVELS.timing]: { + [log.KEYS.timing]: { show: ({ timing, index }) => !!timing && index !== 0, }, } @@ -102,11 +91,13 @@ const setBlocking = (stream) => { return stream } -const getLevel = (stringOrLevelObject) => { - if (typeof stringOrLevelObject === 'string') { - return { level: stringOrLevelObject } +const withMeta = (handler) => (level, ...args) => { + let meta = {} + const last = args.at(-1) + if (last && typeof last === 'object' && Object.hasOwn(last, META)) { + meta = args.pop() } - return stringOrLevelObject + return handler(level, meta, ...args) } class Display { @@ -136,6 +127,7 @@ class Display { #timing #json #heading + #silent // display streams #stdout @@ -205,19 +197,11 @@ class Display { this.#timing = timing this.#json = json this.#heading = heading - - // In silent mode we remove all the handlers - if (this.#levelIndex <= 0) { - this.off() - return - } + this.#silent = this.#levelIndex <= 0 // Emit resume event on the logs which will flush output log.resume() - - // TODO: this should be a proc-log method `proc-log.output.flush`? - this.#outputHandler(OUTPUT_LEVELS.flush) - + output.flush() this.#startProgress({ progress, unicode }) } @@ -236,107 +220,98 @@ class Display { // Arrow function assigned to a private class field so it can be passed // directly as a listener and still reference "this" - #logHandler = (...args) => { - if (args[0] === LOG_LEVELS.resume) { + #logHandler = withMeta((level, meta, ...args) => { + if (level === log.KEYS.resume) { this.#logState.buffering = false this.#logState.buffer.forEach((item) => this.#tryWriteLog(...item)) this.#logState.buffer.length = 0 return } - if (args[0] === LOG_LEVELS.pause) { + if (level === log.KEYS.pause) { this.#logState.buffering = true return } if (this.#logState.buffering) { - this.#logState.buffer.push(args) + this.#logState.buffer.push([level, meta, ...args]) return } - this.#tryWriteLog(...args) - } + this.#tryWriteLog(level, meta, ...args) + }) // Arrow function assigned to a private class field so it can be passed // directly as a listener and still reference "this" - #outputHandler = (...args) => { - if (args[0] === OUTPUT_LEVELS.flush) { + #outputHandler = withMeta((level, meta, ...args) => { + if (level === output.KEYS.flush) { this.#outputState.buffering = false - if (args[1] && this.#json) { + + if (meta.jsonError && this.#json) { const json = {} - for (const [, item] of this.#outputState.buffer) { - Object.assign(json, tryJsonParse(item)) + for (const item of this.#outputState.buffer) { + // index 2 skips the level and meta + Object.assign(json, tryJsonParse(item[2])) } - this.#writeOutput('standard', JSON.stringify({ ...json, ...args[1] }, null, 2)) + this.#writeOutput( + output.KEYS.standard, + meta, + JSON.stringify({ ...json, error: meta.jsonError }, null, 2) + ) } else { this.#outputState.buffer.forEach((item) => this.#writeOutput(...item)) } + this.#outputState.buffer.length = 0 return } - if (args[0] === OUTPUT_LEVELS.buffer) { - this.#outputState.buffer.push(['standard', ...args.slice(1)]) + if (level === output.KEYS.buffer) { + this.#outputState.buffer.push([output.KEYS.standard, meta, ...args]) return } if (this.#outputState.buffering) { - this.#outputState.buffer.push(args) + this.#outputState.buffer.push([level, meta, ...args]) return } - this.#writeOutput(...args) - } + this.#writeOutput(level, meta, ...args) + }) // OUTPUT - #writeOutput (...args) { - const { level } = getLevel(args.shift()) - - if (level === OUTPUT_LEVELS.standard) { + #writeOutput (level, meta, ...args) { + if (level === output.KEYS.standard) { this.#stdoutWrite({}, ...args) return } - if (level === OUTPUT_LEVELS.error) { + if (level === output.KEYS.error) { this.#stderrWrite({}, ...args) } } - // TODO: move this to proc-log and remove this public method - flushOutput (jsonError) { - this.#outputHandler(OUTPUT_LEVELS.flush, jsonError) - } - // LOGS - // TODO: make proc-log able to send signal data like `force` - // when that happens, remove this public method - forceLog (level, ...args) { - // This will show the log regardless of the current loglevel except when silent - if (this.#levelIndex !== 0) { - this.#logHandler({ level, force: true }, ...args) - } - } - - #tryWriteLog (...args) { + #tryWriteLog (level, meta, ...args) { try { // Also (and this is a really inexcusable kludge), we patch the // log.warn() method so that when we see a peerDep override // explanation from Arborist, we can replace the object with a // highly abbreviated explanation of what's being overridden. // TODO: this could probably be moved to arborist now that display is refactored - const [level, heading, message, expl] = args - if (level === LOG_LEVELS.warn && heading === 'ERESOLVE' && expl && typeof expl === 'object') { - this.#writeLog(level, heading, message) - this.#writeLog(level, '', explain(expl, this.#stderrChalk, 2)) + const [heading, message, expl] = args + if (level === log.KEYS.warn && heading === 'ERESOLVE' && expl && typeof expl === 'object') { + this.#writeLog(level, meta, heading, message) + this.#writeLog(level, meta, '', explain(expl, this.#stderrChalk, 2)) return } - this.#writeLog(...args) + this.#writeLog(level, meta, ...args) } catch (ex) { try { // if it crashed once, it might again! - this.#writeLog(LOG_LEVELS.verbose, null, `attempt to log crashed`, ...args, ex) + this.#writeLog(log.KEYS.verbose, meta, '', `attempt to log crashed`, ...args, ex) } catch (ex2) { // This happens if the object has an inspect method that crashes so just console.error // with the errors but don't do anything else that might error again. @@ -346,11 +321,10 @@ class Display { } } - #writeLog (...args) { - const { level, force = false } = getLevel(args.shift()) - + #writeLog (level, meta, ...args) { const levelOpts = LEVEL_METHODS[level] const show = levelOpts.show ?? (({ index }) => levelOpts.index <= index) + const force = meta.force && !this.#silent if (force || show({ index: this.#levelIndex, timing: this.#timing })) { // this mutates the array so we can pass args directly to format later @@ -369,7 +343,7 @@ class Display { // PROGRESS #startProgress ({ progress, unicode }) { - if (!progress) { + if (!progress || this.#silent) { return } this.#progress = proggy.createClient({ normalize: true }) diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index 5d25734f08ad1..969e56636dfe8 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -8,11 +8,9 @@ const messageText = msg => msg.map(line => line.slice(1).join(' ')).join('\n') const jsonError = (er, npm, { summary, detail }) => { if (npm?.config.loaded && npm.config.get('json')) { return { - error: { - code: er.code, - summary: messageText(summary), - detail: messageText(detail), - }, + code: er.code, + summary: messageText(summary), + detail: messageText(detail), } } } diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js index 466b9b2a0ba46..d1879da7f845e 100644 --- a/test/fixtures/mock-npm.js +++ b/test/fixtures/mock-npm.js @@ -2,6 +2,7 @@ const os = require('os') const fs = require('fs').promises const path = require('path') const tap = require('tap') +const { output, META } = require('proc-log') const errorMessage = require('../../lib/utils/error-message') const mockLogs = require('./mock-logs.js') const mockGlobals = require('@npmcli/mock-globals') @@ -79,7 +80,8 @@ const getMockNpm = async (t, { mocks, init, load, npm: npmOpts }) => { // error message fn. This is necessary for commands with buffered output // to read the output after exec is called. This is not *exactly* how it // works in practice, but it is close enough for now. - this.flushOutput(err ? errorMessage(err, this).json : null) + const jsonError = err && errorMessage(err, this).json + output.flush({ [META]: true, jsonError }) if (err) { throw err } diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index b1f4d9c272533..d7bedd7341632 100644 --- a/test/lib/cli/exit-handler.js +++ b/test/lib/cli/exit-handler.js @@ -78,11 +78,9 @@ const mockExitHandler = async (t, { config, mocks, files, ...opts } = {}) => { detail: [['ERR DETAIL', err.message]], ...(files ? { files } : {}), json: { - error: { - code: err.code, - summary: err.message, - detail: err.message, - }, + code: err.code, + summary: err.message, + detail: err.message, }, }), ...mocks, From 3f473d244ba1d8628e382772678795e38d896283 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 16:48:19 -0700 Subject: [PATCH 10/16] chore: test for early version exit --- test/lib/cli/entry.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/lib/cli/entry.js b/test/lib/cli/entry.js index feb6692704d2a..bca3315a5d76e 100644 --- a/test/lib/cli/entry.js +++ b/test/lib/cli/entry.js @@ -1,4 +1,5 @@ const t = require('tap') +const { readdirSync } = require('fs') const { dirname } = require('path') const { load: loadMockNpm } = require('../../fixtures/mock-npm.js') const tmock = require('../../fixtures/tmock.js') @@ -170,3 +171,14 @@ t.test('non-ascii dash', async t => { 'arg Argument starts with non-ascii dash, this is probably invalid: \u2010not-a-dash' ) }) + +t.test('exit early for --version', async t => { + const { cli, outputs, Npm, cache } = await cliMock(t, { + globals: { + 'process.argv': ['node', 'npm', '-v'], + }, + }) + await cli(process) + t.strictSame(readdirSync(cache), [], 'nothing created in cache') + t.equal(outputs[0], Npm.version) +}) From 9a168fa99d80c621d2fe7bc93a5a61d3c2f06c0f Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 17:02:29 -0700 Subject: [PATCH 11/16] fix: dont expose as many public properties of timers --- lib/cli/exit-handler.js | 2 +- lib/npm.js | 7 +++-- lib/utils/timers.js | 61 ++++++++++++++++++++------------------- test/fixtures/mock-npm.js | 2 +- test/lib/npm.js | 41 ++------------------------ test/lib/utils/timers.js | 45 ++++++++--------------------- 6 files changed, 53 insertions(+), 105 deletions(-) diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js index 8d1d5a3e0ef34..7d81e79356748 100644 --- a/lib/cli/exit-handler.js +++ b/lib/cli/exit-handler.js @@ -45,7 +45,7 @@ process.on('exit', code => { // write the timing file now, this might do nothing based on the configs set. // we need to call it here in case it errors so we dont tell the user // about a timing file that doesn't exist - npm.writeTimingFile() + npm.finish() const logsDir = npm.logsDir const logFiles = npm.logFiles diff --git a/lib/npm.js b/lib/npm.js index e0f3f40a72874..1deae661bc100 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -38,7 +38,7 @@ class Npm { #display = null #logFile = new LogFile() - #timers = new Timers({ start: 'npm' }) + #timers = new Timers() // all these options are only used by tests in order to make testing more // closely resemble real world usage. for now, npm has no programmatic API so @@ -117,8 +117,8 @@ class Npm { this.#logFile.off() } - writeTimingFile () { - this.#timers.writeFile({ + finish () { + this.#timers.finish({ id: this.#runId, command: this.#argvClean, logfiles: this.logFiles, @@ -209,6 +209,7 @@ class Npm { this.#timers.load({ path: this.config.get('timing') ? this.logPath : null, + timing: this.config.get('timing'), }) const configScope = this.config.get('scope') diff --git a/lib/utils/timers.js b/lib/utils/timers.js index b27ddd588db06..16a255961fee3 100644 --- a/lib/utils/timers.js +++ b/lib/utils/timers.js @@ -5,7 +5,8 @@ const { log, time } = require('proc-log') const INITIAL_TIMER = 'npm' class Timers extends EE { - file = null + #file + #timing #unfinished = new Map() #finished = {} @@ -17,14 +18,6 @@ class Timers extends EE { this.started = this.#unfinished.get(INITIAL_TIMER) } - get unfinished () { - return this.#unfinished - } - - get finished () { - return this.#finished - } - on () { process.on('time', this.#timeHandler) } @@ -33,36 +26,46 @@ class Timers extends EE { process.off('time', this.#timeHandler) } - load ({ path } = {}) { - if (path) { - this.file = `${path}timing.json` - } + load ({ path, timing } = {}) { + this.#timing = timing + this.#file = `${path}timing.json` } - writeFile (metadata) { - if (!this.file) { + finish (metadata) { + time.end(INITIAL_TIMER) + + for (const [name, timer] of this.#unfinished) { + log.silly('unfinished npm timer', name, timer) + } + + if (!this.#timing) { + // Not in timing mode, nothing else to do here return } try { - const globalStart = this.started - const globalEnd = this.#finished[INITIAL_TIMER] || Date.now() - const content = { - metadata, - timers: this.#finished, - // add any unfinished timers with their relative start/end - unfinishedTimers: [...this.#unfinished.entries()].reduce((acc, [name, start]) => { - acc[name] = [start - globalStart, globalEnd - globalStart] - return acc - }, {}), - } - fs.writeFileSync(this.file, JSON.stringify(content) + '\n') + this.#writeFile(metadata) + log.info('timing', `Timing info written to: ${this.#file}`) } catch (e) { - this.file = null log.warn('timing', `could not write timing file: ${e}`) } } + #writeFile (metadata) { + const globalStart = this.started + const globalEnd = this.#finished[INITIAL_TIMER] + const content = { + metadata, + timers: this.#finished, + // add any unfinished timers with their relative start/end + unfinishedTimers: [...this.#unfinished.entries()].reduce((acc, [name, start]) => { + acc[name] = [start - globalStart, globalEnd - globalStart] + return acc + }, {}), + } + fs.writeFileSync(this.#file, JSON.stringify(content) + '\n') + } + #timeHandler = (level, name) => { const now = Date.now() switch (level) { @@ -76,7 +79,7 @@ class Timers extends EE { this.#unfinished.delete(name) log.timing(name, `Completed in ${ms}ms`) } else { - log.silly('timing', "Tried to end timer that doesn't exist:", name) + log.silly('timing', `Tried to end timer that doesn't exist: ${name}`) } } } diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js index d1879da7f845e..4a58b869858fd 100644 --- a/test/fixtures/mock-npm.js +++ b/test/fixtures/mock-npm.js @@ -295,7 +295,7 @@ const setupMockNpm = async (t, { .join('\n') }, timingFile: async () => { - const data = await fs.readFile(npm.timingFile, 'utf8') + const data = await fs.readFile(npm.logPath + 'timing.json', 'utf8') return JSON.parse(data) }, } diff --git a/test/lib/npm.js b/test/lib/npm.js index 915e63d0060bc..63ac48958cf44 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -380,48 +380,14 @@ t.test('cache dir', async t => { }) t.test('timings', async t => { - t.test('gets/sets timers', async t => { - const { npm, logs } = await loadMockNpm(t, { - config: { - timing: true, - }, - }) - time.start('foo') - time.start('bar') - t.match(npm.unfinishedTimers.get('foo'), Number, 'foo timer is a number') - t.match(npm.unfinishedTimers.get('bar'), Number, 'foo timer is a number') - time.end('foo') - time.end('bar') - time.end('baz') - // npm timer is started by default - time.end('npm') - t.match(logs.timing.byTitle('foo'), [ - /Completed in [0-9]+ms/, - ]) - t.match(logs.timing.byTitle('bar'), [ - /Completed in [0-9]+ms/, - ]) - t.match(logs.timing.byTitle('npm'), [ - /Completed in [0-9]+ms/, - ]) - t.match(logs.silly.byTitle('timing'), [ - `timing Tried to end timer that doesn't exist: baz`, - ]) - t.notOk(npm.unfinishedTimers.has('foo'), 'foo timer is gone') - t.notOk(npm.unfinishedTimers.has('bar'), 'bar timer is gone') - t.match(npm.finishedTimers, { foo: Number, bar: Number, npm: Number }) - }) - t.test('writes timings file', async t => { - const { npm, cache, timingFile } = await loadMockNpm(t, { + const { npm, timingFile } = await loadMockNpm(t, { config: { timing: true }, }) time.start('foo') time.end('foo') time.start('bar') - npm.writeTimingFile() - t.match(npm.timingFile, cache) - t.match(npm.timingFile, /-timing.json$/) + npm.finish() const timings = await timingFile() t.match(timings, { metadata: { @@ -431,7 +397,6 @@ t.test('timings', async t => { }, unfinishedTimers: { bar: [Number, Number], - npm: [Number, Number], }, timers: { foo: Number, @@ -444,7 +409,7 @@ t.test('timings', async t => { const { npm, timingFile } = await loadMockNpm(t, { config: { timing: false }, }) - npm.writeTimingFile() + npm.finish() await t.rejects(() => timingFile()) }) diff --git a/test/lib/utils/timers.js b/test/lib/utils/timers.js index 1901738feb975..4e5bfb104db97 100644 --- a/test/lib/utils/timers.js +++ b/test/lib/utils/timers.js @@ -4,7 +4,7 @@ const fs = require('graceful-fs') const { log, time } = require('proc-log') const tmock = require('../../fixtures/tmock') -const mockTimers = (t, options) => { +const mockTimers = (t) => { const logs = log.LEVELS.reduce((acc, l) => { acc[l] = [] return acc @@ -14,7 +14,7 @@ const mockTimers = (t, options) => { } process.on('log', logHandler) const Timers = tmock(t, '{LIB}/utils/timers') - const timers = new Timers(options) + const timers = new Timers() t.teardown(() => { timers.off() process.off('log', logHandler) @@ -22,32 +22,12 @@ const mockTimers = (t, options) => { return { timers, logs } } -t.test('listens/stops on process', async (t) => { - const { timers } = mockTimers(t) - time.start('foo') - time.start('bar') - time.end('bar') - t.match(timers.unfinished, new Map([['foo', Number]])) - t.match(timers.finished, { bar: Number }) - timers.off() - time.start('baz') - t.notOk(timers.unfinished.get('baz')) -}) - -t.test('initial timer is named npm', async (t) => { - const { timers } = mockTimers(t) - time.end('npm') - t.match(timers.finished, { npm: Number }) -}) - t.test('logs timing events', async (t) => { - const events = [] - const listener = (...args) => events.push(args) - const { timers, logs } = mockTimers(t, { listener }) + const { timers, logs } = mockTimers(t) time.start('foo') time.start('bar') time.end('bar') - timers.off(listener) + timers.off() time.end('foo') t.equal(logs.timing.length, 1) t.match(logs.timing[0], /^bar Completed in [0-9]ms/) @@ -64,14 +44,15 @@ t.test('writes file', async (t) => { const dir = t.testdir() time.start('foo') time.end('foo') - timers.load({ path: resolve(dir, `TIMING_FILE-`) }) - timers.writeFile({ some: 'data' }) + time.start('ohno') + timers.load({ timing: true, path: resolve(dir, `TIMING_FILE-`) }) + timers.finish({ some: 'data' }) const data = JSON.parse(fs.readFileSync(resolve(dir, 'TIMING_FILE-timing.json'))) t.match(data, { metadata: { some: 'data' }, - timers: { foo: Number }, + timers: { foo: Number, npm: Number }, unfinishedTimers: { - npm: [Number, Number], + ohno: [Number, Number], }, }) }) @@ -80,20 +61,18 @@ t.test('fails to write file', async (t) => { const { logs, timers } = mockTimers(t) const dir = t.testdir() - timers.load({ path: join(dir, 'does', 'not', 'exist') }) - timers.writeFile() + timers.load({ timing: true, path: join(dir, 'does', 'not', 'exist') }) + timers.finish() t.match(logs.warn, ['timing could not write timing file:']) - t.equal(timers.file, null) }) t.test('no dir and no file', async (t) => { const { logs, timers } = mockTimers(t) timers.load() - timers.writeFile() + timers.finish() t.strictSame(logs.warn, []) t.strictSame(logs.silly, []) - t.equal(timers.file, null) }) From 695d74efa59eb667ca54169631035dd394ee231d Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 19:32:56 -0700 Subject: [PATCH 12/16] fix: dont show run script banners in silent --- lib/utils/display.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/utils/display.js b/lib/utils/display.js index 32399a7bf5329..4e0ba81ca956e 100644 --- a/lib/utils/display.js +++ b/lib/utils/display.js @@ -276,6 +276,13 @@ class Display { return } + // HACK: if it looks like the banner and we are silent do not print it. + // There's no other way to do this right now :( + // eslint-disable-next-line max-len + if (this.#silent && args.length === 1 && args[0].startsWith('\n> ') && args[0].endsWith('\n')) { + return + } + this.#writeOutput(level, meta, ...args) }) From 9a9d01543fa9d34d5c80ca7e9b46d581a74cbbbe Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 19:33:45 -0700 Subject: [PATCH 13/16] fix(refactor): move timer and error logfile messages to lib/npm --- lib/cli/exit-handler.js | 70 ++----------------- lib/npm.js | 44 +++++++----- .../test/lib/cli/exit-handler.js.test.cjs | 8 +-- .../test/lib/utils/exit-handler.js.test.cjs | 68 ------------------ test/lib/cli/exit-handler.js | 17 ++--- 5 files changed, 39 insertions(+), 168 deletions(-) delete mode 100644 tap-snapshots/test/lib/utils/exit-handler.js.test.cjs diff --git a/lib/cli/exit-handler.js b/lib/cli/exit-handler.js index 7d81e79356748..5866c46b57c5f 100644 --- a/lib/cli/exit-handler.js +++ b/lib/cli/exit-handler.js @@ -1,4 +1,4 @@ -const { log, output, time, META } = require('proc-log') +const { log, output, META } = require('proc-log') const errorMessage = require('../utils/error-message.js') const { redactLog: replaceInfo } = require('@npmcli/redact') @@ -7,19 +7,8 @@ let exitHandlerCalled = false let showLogFileError = false process.on('exit', code => { - // process.emit is synchronous, so the timeEnd handler will run before the - // unfinished timer check below - time.end('npm') - const hasLoadedNpm = npm?.config.loaded - // Unfinished timers can be read before config load - if (npm) { - for (const [name, timer] of npm.unfinishedTimers) { - log.silly('unfinished npm timer', name, timer) - } - } - if (!code) { log.info('ok') } else { @@ -31,65 +20,14 @@ process.on('exit', code => { log.error('', 'Exit handler never called!') log.error('', 'This is an error with npm itself. Please report this error at:') log.error('', ' ') - - if (!npm || !npm.silent) { - // eslint-disable-next-line no-console - console.error('') - } - + // eslint-disable-next-line no-console + console.error('') showLogFileError = true } // npm must be loaded to know where the log file was written if (hasLoadedNpm) { - // write the timing file now, this might do nothing based on the configs set. - // we need to call it here in case it errors so we dont tell the user - // about a timing file that doesn't exist - npm.finish() - - const logsDir = npm.logsDir - const logFiles = npm.logFiles - - const timingDir = npm.timingDir - const timingFile = npm.timingFile - - const timing = npm.config.get('timing') - const logsMax = npm.config.get('logs-max') - - // Determine whether to show log file message and why it is - // being shown since in timing mode we always show the log file message - const logMethod = showLogFileError ? 'error' : timing ? 'info' : null - - if (logMethod) { - // just a line break, will be ignored in silent mode - output.error('') - - const message = [] - - if (timingFile) { - message.push(`Timing info written to: ${timingFile}`) - } else if (timing) { - message.push( - `The timing file was not written due to an error writing to the directory: ${timingDir}` - ) - } - - if (logFiles.length) { - message.push(`A complete log of this run can be found in: ${logFiles}`) - } else if (logsMax <= 0) { - // user specified no log file - message.push(`Log files were not written due to the config logs-max=${logsMax}`) - } else { - // could be an error writing to the directory - message.push( - `Log files were not written due to an error writing to the directory: ${logsDir}`, - 'You can rerun the command with `--loglevel=verbose` to see the logs in your terminal' - ) - } - - log[logMethod]('', message.join('\n')) - } - + npm.finish({ showLogFileError }) // This removes any listeners npm setup, mostly for tests to avoid max listener warnings npm.unload() } diff --git a/lib/npm.js b/lib/npm.js index 1deae661bc100..691d13ef1be99 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -117,13 +117,35 @@ class Npm { this.#logFile.off() } - finish () { + finish ({ showLogFileError } = {}) { this.#timers.finish({ id: this.#runId, command: this.#argvClean, logfiles: this.logFiles, version: this.version, }) + + if (showLogFileError) { + if (!this.silent) { + // just a line break if not in silent mode + output.error('') + } + + const logsMax = this.config.get('logs-max') + + if (this.logFiles.length) { + log.error('', `A complete log of this run can be found in: ${this.logFiles}`) + } else if (logsMax <= 0) { + // user specified no log file + log.error('', `Log files were not written due to the config logs-max=${logsMax}`) + } else { + // could be an error writing to the directory + log.error('', + `Log files were not written due to an error writing to the directory: ${this.#logsDir}` + + '\nYou can rerun the command with `--loglevel=verbose` to see the logs in your terminal' + ) + } + } } get title () { @@ -179,7 +201,7 @@ class Npm { // which we will tell them about at the end if (this.config.get('logs-max') > 0) { await time.start('npm:load:mkdirplogs', () => - fs.mkdir(this.logsDir, { recursive: true }) + fs.mkdir(this.#logsDir, { recursive: true }) .catch((e) => log.verbose('logfile', `could not create logs-dir: ${e}`))) } @@ -208,7 +230,7 @@ class Npm { }) this.#timers.load({ - path: this.config.get('timing') ? this.logPath : null, + path: this.logPath, timing: this.config.get('timing'), }) @@ -281,14 +303,6 @@ class Npm { return 2 } - get unfinishedTimers () { - return this.#timers.unfinished - } - - get finishedTimers () { - return this.#timers.finished - } - get started () { return this.#timers.started } @@ -297,16 +311,12 @@ class Npm { return this.#logFile.files } - get logsDir () { + get #logsDir () { return this.config.get('logs-dir') || join(this.cache, '_logs') } get logPath () { - return resolve(this.logsDir, `${this.#runId}-`) - } - - get timingFile () { - return this.#timers.file + return resolve(this.#logsDir, `${this.#runId}-`) } get npmRoot () { diff --git a/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs b/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs index 3a510647ece8e..cedb56642f26d 100644 --- a/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs +++ b/tap-snapshots/test/lib/cli/exit-handler.js.test.cjs @@ -29,9 +29,9 @@ XX error code ECODE XX error ERR SUMMARY Unknown error XX error ERR DETAIL Unknown error XX verbose exit 1 -XX timing npm Completed in {TIME}ms XX verbose code 1 -XX error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json +XX timing npm Completed in {TIME}ms +XX info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json XX error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log ` @@ -59,8 +59,8 @@ error code ECODE error ERR SUMMARY Unknown error error ERR DETAIL Unknown error verbose exit 1 -timing npm Completed in {TIME}ms verbose code 1 -error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json +timing npm Completed in {TIME}ms +info timing Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log ` diff --git a/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs b/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs deleted file mode 100644 index 3fa13309d6249..0000000000000 --- a/tap-snapshots/test/lib/utils/exit-handler.js.test.cjs +++ /dev/null @@ -1,68 +0,0 @@ -/* IMPORTANT - * This snapshot file is auto-generated, but designed for humans. - * It should be checked into source control and tracked carefully. - * Re-generate by setting TAP_SNAPSHOT=1 and running tests. - * Make sure to inspect the output below. Do not ignore changes! - */ -'use strict' -exports[`test/lib/utils/exit-handler.js TAP handles unknown error with logs and debug file > debug file contents 1`] = ` -XX timing npm:load:whichnode Completed in {TIME}ms -XX timing config:load Completed in {TIME}ms -XX timing npm:load:configload Completed in {TIME}ms -XX timing npm:load:mkdirpcache Completed in {TIME}ms -XX timing npm:load:mkdirplogs Completed in {TIME}ms -XX verbose title npm -XX verbose argv "--fetch-retries" "0" "--cache" "{CWD}/cache" "--loglevel" "silly" "--color" "false" "--timing" "true" -XX timing npm:load:setTitle Completed in {TIME}ms -XX timing npm:load:display Completed in {TIME}ms -XX verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}- -XX verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log -XX timing npm:load:logFile Completed in {TIME}ms -XX timing npm:load:timers Completed in {TIME}ms -XX timing npm:load:configScope Completed in {TIME}ms -XX timing npm:load Completed in {TIME}ms -XX verbose stack Error: Unknown error -XX verbose cwd {CWD}/prefix -XX verbose {OS} -XX verbose {NODE-VERSION} -XX verbose npm {NPM-VERSION} -XX error code ECODE -XX error ERR SUMMARY Unknown error -XX error ERR DETAIL Unknown error -XX verbose exit 1 -XX timing npm Completed in {TIME}ms -XX verbose code 1 -XX error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json -XX error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log -` - -exports[`test/lib/utils/exit-handler.js TAP handles unknown error with logs and debug file > logs 1`] = ` -timing npm:load:whichnode Completed in {TIME}ms -timing config:load Completed in {TIME}ms -timing npm:load:configload Completed in {TIME}ms -timing npm:load:mkdirpcache Completed in {TIME}ms -timing npm:load:mkdirplogs Completed in {TIME}ms -verbose title npm -verbose argv "--fetch-retries" "0" "--cache" "{CWD}/cache" "--loglevel" "silly" "--color" "false" "--timing" "true" -timing npm:load:setTitle Completed in {TIME}ms -timing npm:load:display Completed in {TIME}ms -verbose logfile logs-max:10 dir:{CWD}/cache/_logs/{DATE}- -verbose logfile {CWD}/cache/_logs/{DATE}-debug-0.log -timing npm:load:logFile Completed in {TIME}ms -timing npm:load:timers Completed in {TIME}ms -timing npm:load:configScope Completed in {TIME}ms -timing npm:load Completed in {TIME}ms -verbose stack Error: Unknown error -verbose cwd {CWD}/prefix -verbose {OS} -verbose {NODE-VERSION} -verbose npm {NPM-VERSION} -error code ECODE -error ERR SUMMARY Unknown error -error ERR DETAIL Unknown error -verbose exit 1 -timing npm Completed in {TIME}ms -verbose code 1 -error Timing info written to: {CWD}/cache/_logs/{DATE}-timing.json -error A complete log of this run can be found in: {CWD}/cache/_logs/{DATE}-debug-0.log -` diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index d7bedd7341632..3cb4523b3ee51 100644 --- a/test/lib/cli/exit-handler.js +++ b/test/lib/cli/exit-handler.js @@ -382,7 +382,7 @@ t.test('timers fail to write', async (t) => { await exitHandler(new Error()) - t.match(logs.error[2], `error writing to the directory`) + t.match(logs.warn[0], `timing could not write timing file: Error: err`) }) t.test('log files fail to write', async (t) => { @@ -455,7 +455,7 @@ t.test('files from error message with error', async (t) => { t.test('timing with no error', async (t) => { const { exitHandler, timingFile, npm, logs } = await mockExitHandler(t, { - config: { timing: true, loglevel: 'info' }, + config: { timing: true, loglevel: 'silly' }, }) await exitHandler() @@ -463,18 +463,9 @@ t.test('timing with no error', async (t) => { t.equal(process.exitCode, 0) - const msg = logs.info[1] - t.match(msg, /A complete log of this run can be found in:/) + const msg = logs.info.byTitle('timing')[0] t.match(msg, /Timing info written to:/) - t.match( - timingFileData.timers, - Object.keys(npm.finishedTimers).reduce((acc, k) => { - acc[k] = Number - return acc - }, {}) - ) - t.strictSame(npm.unfinishedTimers, new Map()) t.match(timingFileData, { metadata: { command: [], @@ -482,6 +473,7 @@ t.test('timing with no error', async (t) => { logfiles: [String], }, timers: { + 'npm:load': Number, npm: Number, }, }) @@ -511,7 +503,6 @@ t.test('unfinished timers', async (t) => { const timingFileData = await timingFile() t.equal(process.exitCode, 0) - t.match(npm.unfinishedTimers, new Map([['foo', Number], ['bar', Number]])) t.match(timingFileData, { metadata: { command: [], From bcbebac04a3faa9d203b2610315c63a75d14442a Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 19:49:13 -0700 Subject: [PATCH 14/16] fix: run update notifier after exec but before waiting --- lib/cli/entry.js | 29 ++++++++++++++++------------- lib/npm.js | 13 +++++++------ lib/utils/did-you-mean.js | 16 +++++++--------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/lib/cli/entry.js b/lib/cli/entry.js index 9b3e29ec336a6..db61dd74f56b0 100644 --- a/lib/cli/entry.js +++ b/lib/cli/entry.js @@ -36,7 +36,6 @@ module.exports = async (process, validateEngines) => { log.warn('cli', validateEngines.unsupportedMessage) } - let cmd // Now actually fire up npm and run the command. // This is how to use npm programmatically: try { @@ -46,20 +45,15 @@ module.exports = async (process, validateEngines) => { return exitHandler() } - cmd = npm.argv.shift() - if (!cmd) { + const command = npm.argv.shift() + const args = npm.argv + + if (!command) { output.standard(npm.usage) process.exitCode = 1 return exitHandler() } - // this is async but we dont await it, since its ok if it doesnt - // finish before the command finishes running. it uses command and argv - // so it must be initiated here, after the command name is set - const updateNotifier = require('./update-notifier.js') - // eslint-disable-next-line promise/catch-or-return - updateNotifier(npm).then((msg) => (npm.updateNotification = msg)) - // Options are prefixed by a hyphen-minus (-, \u2d). // Other dash-type chars look similar but are invalid. const nonDashArgs = npm.argv.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a)) @@ -71,13 +65,22 @@ module.exports = async (process, validateEngines) => { ) } - await npm.exec(cmd) + const execPromise = npm.exec(command, args) + + // this is async but we dont await it, since its ok if it doesnt + // finish before the command finishes running. it uses command and argv + // so it must be initiated here, after the command name is set + const updateNotifier = require('./update-notifier.js') + // eslint-disable-next-line promise/catch-or-return + updateNotifier(npm).then((msg) => (npm.updateNotification = msg)) + + await execPromise return exitHandler() } catch (err) { if (err.code === 'EUNKNOWNCOMMAND') { const didYouMean = require('../utils/did-you-mean.js') - const suggestions = await didYouMean(npm.localPrefix, cmd) - output.standard(`Unknown command: "${cmd}"${suggestions}\n`) + const suggestions = await didYouMean(npm.localPrefix, err.command) + output.standard(`Unknown command: "${err.command}"${suggestions}\n`) output.standard('To see a list of supported npm commands, run:\n npm help') process.exitCode = 1 return exitHandler() diff --git a/lib/npm.js b/lib/npm.js index 691d13ef1be99..9b00321556a44 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -22,6 +22,7 @@ class Npm { if (!command) { throw Object.assign(new Error(`Unknown command ${c}`), { code: 'EUNKNOWNCOMMAND', + command: c, }) } return require(`./commands/${command}.js`) @@ -90,18 +91,18 @@ class Npm { } // Call an npm command - // TODO: tests are currently the only time the second - // parameter of args is used. When called via `lib/cli.js` the config is - // loaded and this.argv is set to the remaining command line args. We should - // consider testing the CLI the same way it is used and not allow args to be - // passed in directly. async exec (cmd, args = this.argv) { const command = this.setCmd(cmd) return time.start(`command:${cmd}`, () => command.cmdExec(args)) } async load () { - return time.start('npm:load', () => this.#load().then(r => r || { exec: true })) + return time.start('npm:load', async () => { + const { exec = true } = await this.#load().then(r => r ?? {}) + return { + exec, + } + }) } get loaded () { diff --git a/lib/utils/did-you-mean.js b/lib/utils/did-you-mean.js index 499c27c74e1de..54c8ff2e35aa6 100644 --- a/lib/utils/did-you-mean.js +++ b/lib/utils/did-you-mean.js @@ -8,7 +8,7 @@ const didYouMean = async (path, scmd) => { let best = [] for (const str of close) { const cmd = Npm.cmd(str) - best.push(` npm ${str} # ${cmd.description}`) + best.push(` npm ${str} # ${cmd.description}`) } // We would already be suggesting this in `npm x` so omit them here const runScripts = ['stop', 'start', 'test', 'restart'] @@ -17,13 +17,13 @@ const didYouMean = async (path, scmd) => { best = best.concat( Object.keys(scripts || {}) .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4 && !runScripts.includes(cmd)) - .map(str => ` npm run ${str} # run the "${str}" package script`), + .map(str => ` npm run ${str} # run the "${str}" package script`), Object.keys(bin || {}) .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4) /* eslint-disable-next-line max-len */ - .map(str => ` npm exec ${str} # run the "${str}" command from either this or a remote npm package`) + .map(str => ` npm exec ${str} # run the "${str}" command from either this or a remote npm package`) ) - } catch (_) { + } catch { // gracefully ignore not being in a folder w/ a package.json } @@ -31,11 +31,9 @@ const didYouMean = async (path, scmd) => { return '' } - const suggestion = - best.length === 1 - ? `\n\nDid you mean this?\n${best[0]}` - : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}` - return suggestion + return best.length === 1 + ? `\n\nDid you mean this?\n${best[0]}` + : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}` } module.exports = didYouMean From e9677f1b1b4d7a50295a48db91263262e9be1c94 Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 22 Apr 2024 19:52:03 -0700 Subject: [PATCH 15/16] fix: store unref promises for awaiting in tests --- lib/npm.js | 10 ++++++++-- test/fixtures/mock-npm.js | 11 +++++++++++ test/lib/cli/exit-handler.js | 9 +-------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/npm.js b/lib/npm.js index 9b00321556a44..892cf4b96c8a0 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -28,6 +28,7 @@ class Npm { return require(`./commands/${command}.js`) } + unrefPromises = [] updateNotification = null argv = [] @@ -224,11 +225,16 @@ class Npm { log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' ')) }) - this.#logFile.load({ + // logFile.load returns a promise that resolves when old logs are done being cleaned. + // We save this promise to an array so that we can await it in tests to ensure more + // deterministic logging behavior. The process will also hang open if this were to + // take a long time to resolve, but that is why process.exit is called explicitly + // in the exit-handler. + this.unrefPromises.push(this.#logFile.load({ path: this.logPath, logsMax: this.config.get('logs-max'), timing: this.config.get('timing'), - }) + })) this.#timers.load({ path: this.logPath, diff --git a/test/fixtures/mock-npm.js b/test/fixtures/mock-npm.js index 4a58b869858fd..5618a12566003 100644 --- a/test/fixtures/mock-npm.js +++ b/test/fixtures/mock-npm.js @@ -73,6 +73,17 @@ const getMockNpm = async (t, { mocks, init, load, npm: npmOpts }) => { }) } + async load () { + const res = await super.load() + // Wait for any promises (currently only log file cleaning) to be + // done before returning from load in tests. This helps create more + // deterministic testing behavior because in reality that promise + // is left hanging on purpose as a best-effort and the process gets + // closed regardless of if it has finished or not. + await Promise.all(this.unrefPromises) + return res + } + async exec (...args) { const [res, err] = await super.exec(...args).then((r) => [r]).catch(e => [null, e]) // This mimics how the exit handler flushes output for commands that have diff --git a/test/lib/cli/exit-handler.js b/test/lib/cli/exit-handler.js index 3cb4523b3ee51..fd6754b30096e 100644 --- a/test/lib/cli/exit-handler.js +++ b/test/lib/cli/exit-handler.js @@ -129,8 +129,6 @@ t.test('handles unknown error with logs and debug file', async (t) => { }) await exitHandler(err('Unknown error', 'ECODE')) - // force logfile cleaning logs to happen since those are purposefully not awaited - await require('timers/promises').setTimeout(200) const fileLogs = await debugFile() const fileLines = fileLogs.split('\n') @@ -140,19 +138,14 @@ t.test('handles unknown error with logs and debug file', async (t) => { t.equal(process.exitCode, 1) - let skippedLogs = 0 logs.forEach((logItem, i) => { const logLines = logItem.split('\n').map(l => `${i} ${l}`) for (const line of logLines) { - if (line.includes('logfile') && line.includes('cleaning')) { - skippedLogs++ - continue - } t.match(fileLogs.trim(), line, 'log appears in debug file') } }) - t.equal(logs.length - skippedLogs, parseInt(lastLog) + 1) + t.equal(logs.length, parseInt(lastLog) + 1) t.match(logs.error, [ 'code ECODE', 'ERR SUMMARY Unknown error', From 439ac0c7f32664f1c047b1550678598628c62cec Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Wed, 24 Apr 2024 13:49:51 -0700 Subject: [PATCH 16/16] fix: redact when displaying non-ascii arguments --- lib/cli/entry.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cli/entry.js b/lib/cli/entry.js index db61dd74f56b0..fc196b8e234fc 100644 --- a/lib/cli/entry.js +++ b/lib/cli/entry.js @@ -61,7 +61,7 @@ module.exports = async (process, validateEngines) => { log.error( 'arg', 'Argument starts with non-ascii dash, this is probably invalid:', - nonDashArgs.join(', ') + require('@npmcli/redact').redactLog(nonDashArgs.join(', ')) ) }