From b51bc7ca2dcdd60b8de5a9c1c9367819a6ccf7cf Mon Sep 17 00:00:00 2001 From: reggi Date: Mon, 24 Nov 2025 09:16:02 -0500 Subject: [PATCH 01/12] chore: command config --- lib/base-cmd.js | 34 ++- lib/commands/config.js | 2 +- lib/commands/publish.js | 2 +- lib/commands/unpublish.js | 2 +- lib/npm.js | 23 +- .../test/lib/commands/install.js.test.cjs | 18 +- workspaces/config/lib/index.js | 207 ++++++++++++++++-- 7 files changed, 248 insertions(+), 40 deletions(-) diff --git a/lib/base-cmd.js b/lib/base-cmd.js index 3e6c4758cbd58..8432355c2c6ff 100644 --- a/lib/base-cmd.js +++ b/lib/base-cmd.js @@ -1,4 +1,5 @@ const { log } = require('proc-log') +const { definitions: globalDefinitions } = require('@npmcli/config/lib/definitions') class BaseCommand { // these defaults can be overridden by individual commands @@ -10,11 +11,11 @@ class BaseCommand { static name = null static description = null static params = null + static definitions = globalDefinitions // this is a static so that we can read from it without instantiating a command // which would require loading the config static get describeUsage () { - const { definitions } = require('@npmcli/config/lib/definitions') const { aliases: cmdAliases } = require('./utils/cmd-list') const seenExclusive = new Set() const wrapWidth = 80 @@ -35,14 +36,14 @@ class BaseCommand { if (seenExclusive.has(param)) { continue } - const { exclusive } = definitions[param] - let paramUsage = `${definitions[param].usage}` + const exclusive = this.definitions[param]?.exclusive + let paramUsage = this.definitions[param]?.usage || '' if (exclusive) { const exclusiveParams = [paramUsage] seenExclusive.add(param) for (const e of exclusive) { seenExclusive.add(e) - exclusiveParams.push(definitions[e].usage) + exclusiveParams.push(this.definitions[e].usage) } paramUsage = `${exclusiveParams.join('|')}` } @@ -77,7 +78,17 @@ class BaseCommand { constructor (npm) { this.npm = npm - const { config } = this.npm + // If this command has custom definitions different from global, create a command-specific config + if (this.definitions !== globalDefinitions) { + this.config = this.npm.createConfig(this.definitions) + this.config.loadSync() + // Warn about unknown configs with custom definitions + if (!this.constructor.skipConfigValidation) { + this.config.warn(this.definitions) + } + } + + const { config } = this if (!this.constructor.skipConfigValidation) { config.validate() @@ -88,10 +99,23 @@ class BaseCommand { } } + get config () { + // Return command-specific config if it exists, otherwise use npm's config + return this._config || this.npm.config + } + + set config (value) { + this._config = value + } + get name () { return this.constructor.name } + get definitions () { + return this.constructor.definitions + } + get description () { return this.constructor.description } diff --git a/lib/commands/config.js b/lib/commands/config.js index b657029b2d2fe..30b3e6ae5690f 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -367,7 +367,7 @@ ${defData} if (content.publishConfig) { for (const key in content.publishConfig) { - this.npm.config.checkUnknown('publishConfig', key) + this.npm.config.checkUnknown('publishConfig', key, true) } const pkgPath = resolve(this.npm.prefix, 'package.json') msg.push(`; "publishConfig" from ${pkgPath}`) diff --git a/lib/commands/publish.js b/lib/commands/publish.js index 162e3d65ba5ce..406b8cd91b942 100644 --- a/lib/commands/publish.js +++ b/lib/commands/publish.js @@ -272,7 +272,7 @@ class Publish extends BaseCommand { Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags))) if (logWarnings) { for (const key in filteredPublishConfig) { - this.npm.config.checkUnknown('publishConfig', key) + this.npm.config.checkUnknown('publishConfig', key, true) } } flatten(filteredPublishConfig, opts) diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index e1c06d3184057..b32818dc75838 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -146,7 +146,7 @@ class Unpublish extends BaseCommand { const filteredPublishConfig = Object.fromEntries( Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags))) for (const key in filteredPublishConfig) { - this.npm.config.checkUnknown('publishConfig', key) + this.npm.config.checkUnknown('publishConfig', key, true) } flatten(filteredPublishConfig, opts) } diff --git a/lib/npm.js b/lib/npm.js index c635f3e05a7b3..6f2fa0897f100 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -2,7 +2,7 @@ const { resolve, dirname, join } = require('node:path') const Config = require('@npmcli/config') const which = require('which') const fs = require('node:fs/promises') -const { definitions, flatten, nerfDarts, shorthands } = require('@npmcli/config/lib/definitions') +const { definitions: globalDefinitions, flatten, nerfDarts, shorthands } = require('@npmcli/config/lib/definitions') const usage = require('./utils/npm-usage.js') const LogFile = require('./utils/log-file.js') const Timers = require('./utils/timers.js') @@ -37,6 +37,8 @@ class Npm { #runId = new Date().toISOString().replace(/[.:]/g, '_') #title = 'npm' #argvClean = [] + #argv = undefined + #excludeNpmCwd = undefined #npmRoot = null #display = null @@ -64,14 +66,20 @@ class Npm { } = {}) { this.#display = new Display({ stdout, stderr }) this.#npmRoot = npmRoot - this.config = new Config({ + this.#argv = argv + this.#excludeNpmCwd = excludeNpmCwd + this.config = this.createConfig(globalDefinitions) + } + + createConfig (definitions) { + return new Config({ npmPath: this.#npmRoot, definitions, flatten, nerfDarts, shorthands, - argv: [...process.argv, ...argv], - excludeNpmCwd, + argv: [...process.argv, ...this.#argv], + excludeNpmCwd: this.#excludeNpmCwd, }) } @@ -227,6 +235,13 @@ class Npm { process.env.npm_command = this.command } + if (Command.definitions === globalDefinitions) { + this.config?.warn() + } else { + const cloned = this.config?.clone() + cloned.loadDefinitions(Command.definitions).warn() + } + if (this.config.get('usage')) { return output.standard(command.usage) } diff --git a/tap-snapshots/test/lib/commands/install.js.test.cjs b/tap-snapshots/test/lib/commands/install.js.test.cjs index 3c9fa9bbec447..02df3c178fd83 100644 --- a/tap-snapshots/test/lib/commands/install.js.test.cjs +++ b/tap-snapshots/test/lib/commands/install.js.test.cjs @@ -134,9 +134,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:181:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:252:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:208:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -199,9 +199,9 @@ warn EBADDEVENGINES } verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:181:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:252:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:208:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -225,9 +225,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:181:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:252:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:208:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 0ad716ccb069f..6c649a893284c 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -243,6 +243,25 @@ class Config { } } + loadSync () { + if (this.loaded) { + throw new Error('attempting to load npm config multiple times') + } + + // first load the defaults, which sets the global prefix + this.loadDefaults() + this.loadCLI() + this.loadEnv() + // 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 + this.#loaded = true + + // set proper globalPrefix now that everything is loaded + this.globalPrefix = this.get('prefix') + + this.setEnvs() + } + async load () { if (this.loaded) { throw new Error('attempting to load npm config multiple times') @@ -563,6 +582,37 @@ class Config { return keyword } + // Parse and set config entries from an object of raw config values + // Used by both #loadObject (initial load) and loadDefinitions (re-parse with new definitions) + #parseConfigEntries (obj, where, configData, filter = null) { + for (const [key, value] of Object.entries(obj)) { + // If filter is provided, only process keys that pass the filter + if (filter && !filter(key)) { + continue + } + + const k = envReplace(key, this.env) + const v = this.parseField(value, k) + + if (where !== 'default') { + this.#checkDeprecated(k) + if (this.definitions[key]?.exclusive) { + for (const exclusive of this.definitions[key].exclusive) { + if (!this.isDefault(exclusive)) { + throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) + } + } + } + } + + if (where !== 'default' || key === 'npm-version') { + this.checkUnknown(where, key, false) + } + + configData.data[k] = v + } + } + #loadObject (obj, where, source, er = null) { // obj is the raw data read from the file const conf = this.data.get(where) @@ -587,28 +637,14 @@ class Config { } } else { conf.raw = obj - for (const [key, value] of Object.entries(obj)) { - const k = envReplace(key, this.env) - const v = this.parseField(value, k) - if (where !== 'default') { - this.#checkDeprecated(k) - if (this.definitions[key]?.exclusive) { - for (const exclusive of this.definitions[key].exclusive) { - if (!this.isDefault(exclusive)) { - throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) - } - } - } - } - if (where !== 'default' || key === 'npm-version') { - this.checkUnknown(where, key) - } - conf.data[k] = v - } + this.#parseConfigEntries(obj, where, conf) } } - checkUnknown (where, key) { + checkUnknown (where, key, emitWarnings = false) { + if (!emitWarnings) { + return + } if (!this.definitions[key]) { if (internalEnv.includes(key)) { return @@ -917,6 +953,139 @@ class Config { return creds } + // Validate unknown configs with additional definitions + // This allows command-specific definitions to be checked against already-loaded config + warn (additionalDefinitions = {}) { + const originalDefs = this.definitions + + // Temporarily merge additional definitions for validation + this.definitions = { ...this.definitions, ...additionalDefinitions } + + try { + // Re-validate all loaded configs with merged definitions + for (const where of ['env', 'cli', 'builtin', 'user', 'project', 'global']) { + const { raw } = this.data.get(where) + if (!raw || !Object.keys(raw).length) { + continue + } + + for (const key of Object.keys(raw)) { + const k = envReplace(key, this.env) + this.checkUnknown(where, k, true) + } + } + } finally { + // Restore original definitions (don't mutate permanently) + this.definitions = originalDefs + } + } + + // Clone this Config instance with all its state + clone () { + const cloned = new Config({ + definitions: { ...this.definitions }, + shorthands: { ...this.shorthands }, + flatten: this.#flatten, + nerfDarts: [...this.nerfDarts], + npmPath: this.npmPath, + env: this.env, + argv: this.argv, + platform: this.platform, + execPath: this.execPath, + cwd: this.cwd, + excludeNpmCwd: this.excludeNpmCwd, + }) + + // Copy loaded state + cloned.#loaded = this.#loaded + cloned.globalPrefix = this.globalPrefix + cloned.localPrefix = this.localPrefix + cloned.localPackage = this.localPackage + cloned.home = this.home + cloned.parsedArgv = this.parsedArgv + + // Deep copy types, defaults, and deprecated to prevent mutation + cloned.types = { ...this.types } + cloned.defaults = { ...this.defaults } + cloned.deprecated = { ...this.deprecated } + + // Copy sources map + cloned.sources = new Map(this.sources) + + // Clone the data map by copying each ConfigData layer + for (const where of confTypes) { + const sourceData = this.data.get(where) + const clonedData = cloned.data.get(where) + + // Copy only own properties from data (not inherited from prototype chain) + for (const key of Object.keys(sourceData.data)) { + clonedData.data[key] = sourceData.data[key] + } + + // Copy raw config + Object.assign(clonedData.raw, sourceData.raw) + + // Copy source (use internal setter to bypass the check) + if (sourceData.source) { + clonedData.source = sourceData.source + } + + // Copy validation state + clonedData[_valid] = sourceData[_valid] + + // Copy load error if any + if (sourceData.loadError) { + clonedData[_loadError] = sourceData.loadError + } + } + + return cloned + } + + // Load additional definitions after config has been loaded + // This re-processes already-loaded config data with the new definitions + loadDefinitions (additionalDefinitions) { + if (!additionalDefinitions) { + return this + } + if (!this.loaded) { + throw new Error('call config.load() or config.loadSync() before loading additional definitions') + } + // Merge new definitions + this.definitions = { ...this.definitions, ...additionalDefinitions } + // Update types, defaults, deprecated maps + for (const [key, def] of Object.entries(additionalDefinitions)) { + this.defaults[key] = def.default + this.types[key] = def.type + if (def.deprecated) { + this.deprecated[key] = def.deprecated.trim().replace(/\n +/, '\n') + } + } + + // Re-process already loaded data with new definitions + for (const where of ['default', 'builtin', 'env', 'cli', 'user', 'project', 'global']) { + const configData = this.data.get(where) + const { raw, source } = configData + + // Skip if this layer wasn't loaded + if (!source || !raw || !Object.keys(raw).length) { + continue + } + + // Re-parse only keys that are in the new definitions + const filter = (key) => { + const k = envReplace(key, this.env) + return additionalDefinitions[k] || additionalDefinitions[key] + } + + this.#parseConfigEntries(raw, where, configData, filter) + } + + // Invalidate flat options since new definitions may affect flattening + this.#flatOptions = null + return this + } + // set up the environment object we have with npm_config_* environs // for all configs that are different from their default values, and // set EDITOR and HOME. From c173f1e330cd9bf073ce47e19d8f928c1349afd6 Mon Sep 17 00:00:00 2001 From: reggi Date: Tue, 25 Nov 2025 16:52:48 -0500 Subject: [PATCH 02/12] refactor using cached promises, warning log gatherer --- lib/base-cmd.js | 18 +- lib/npm.js | 13 +- workspaces/config/lib/index.js | 346 +++++++++++++-------------------- 3 files changed, 153 insertions(+), 224 deletions(-) diff --git a/lib/base-cmd.js b/lib/base-cmd.js index 8432355c2c6ff..69a86ab14ee8d 100644 --- a/lib/base-cmd.js +++ b/lib/base-cmd.js @@ -11,7 +11,7 @@ class BaseCommand { static name = null static description = null static params = null - static definitions = globalDefinitions + static definitions = null // this is a static so that we can read from it without instantiating a command // which would require loading the config @@ -21,6 +21,12 @@ class BaseCommand { const wrapWidth = 80 const { description, usage = [''], name, params } = this + if (this.definitions) { + const cmdDefinitions = this.definitions + this.definitions = { ...globalDefinitions, ...cmdDefinitions } + } else { + this.definitions = globalDefinitions + } const fullUsage = [ `${description}`, '', @@ -78,16 +84,6 @@ class BaseCommand { constructor (npm) { this.npm = npm - // If this command has custom definitions different from global, create a command-specific config - if (this.definitions !== globalDefinitions) { - this.config = this.npm.createConfig(this.definitions) - this.config.loadSync() - // Warn about unknown configs with custom definitions - if (!this.constructor.skipConfigValidation) { - this.config.warn(this.definitions) - } - } - const { config } = this if (!this.constructor.skipConfigValidation) { diff --git a/lib/npm.js b/lib/npm.js index 6f2fa0897f100..f1166efca8c78 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -236,10 +236,15 @@ class Npm { } if (Command.definitions === globalDefinitions) { - this.config?.warn() + this.config.warn() } else { - const cloned = this.config?.clone() - cloned.loadDefinitions(Command.definitions).warn() + const _config = this.config.clone({ + definitions: { ...globalDefinitions, ...Command.definitions }, + shouldSetEnvs: true, + }) + await _config.load() + _config.warn() + command._config = _config } if (this.config.get('usage')) { @@ -338,7 +343,7 @@ class Npm { ...opts, }) - const { writeFileSync } = require('node:fs') + const { writeFileSync, glob } = require('node:fs') for (const [file, content] of files) { const filePath = `${this.logPath}${file}` const fileContent = `'Log files:\n${this.logFiles.join('\n')}\n\n${content.trim()}\n` diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 6c649a893284c..e21ce2364bda5 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -59,12 +59,43 @@ class Config { #flatten // populated the first time we flatten the object #flatOptions = null + // cache for filesystem operations to allow efficient reloading + #fileCache = new Map() + #fileExistsCache = new Map() + #dirExistsCache = new Map() + #pkgJsonCache = new Map() + #warnings = [] static get typeDefs () { return typeDefs } + clone (args) { + return new Config({ + cache: { + fileCache: this.#fileCache, + fileExistsCache: this.#fileExistsCache, + dirExistsCache: this.#dirExistsCache, + pkgJsonCache: this.#pkgJsonCache, + }, + definitions: this.definitions, + shorthands: this.shorthands, + flatten: this.#flatten, + nerfDarts: this.nerfDarts, + npmPath: this.npmPath, + env: this.env, + argv: this.argv, + platform: this.platform, + execPath: this.execPath, + cwd: this.cwd, + excludeNpmCwd: this.excludeNpmCwd, + shouldSetEnvs: this.shouldSetEnvs, + ...args, + }) + } + constructor ({ + cache, definitions, shorthands, flatten, @@ -78,7 +109,16 @@ class Config { execPath = process.execPath, cwd = process.cwd(), excludeNpmCwd = false, + shouldSetEnvs = false, }) { + if (cache) { + this.#fileCache = cache.fileCache + this.#fileExistsCache = cache.fileExistsCache + this.#dirExistsCache = cache.dirExistsCache + this.#pkgJsonCache = cache.pkgJsonCache + } + this.#warnings = [] + this.shouldSetEnvs = shouldSetEnvs this.nerfDarts = nerfDarts this.definitions = definitions // turn the definitions into nopt's weirdo syntax @@ -151,6 +191,12 @@ class Config { return this.#loaded } + warn () { + for (const warning of this.#warnings) { + log.warn(...warning) + } + } + get prefix () { return this.#get('global') ? this.globalPrefix : this.localPrefix } @@ -243,25 +289,6 @@ class Config { } } - loadSync () { - if (this.loaded) { - throw new Error('attempting to load npm config multiple times') - } - - // first load the defaults, which sets the global prefix - this.loadDefaults() - this.loadCLI() - this.loadEnv() - // 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 - this.#loaded = true - - // set proper globalPrefix now that everything is loaded - this.globalPrefix = this.get('prefix') - - this.setEnvs() - } - async load () { if (this.loaded) { throw new Error('attempting to load npm config multiple times') @@ -293,7 +320,9 @@ class Config { // set proper globalPrefix now that everything is loaded this.globalPrefix = this.get('prefix') - this.setEnvs() + if (this.shouldSetEnvs) { + setEnvs(this) + } } loadDefaults () { @@ -383,13 +412,13 @@ class Config { loadCLI () { for (const s of Object.keys(this.shorthands)) { if (s.length > 1 && this.argv.includes(`-${s}`)) { - log.warn(`-${s} is not a valid single-hyphen cli flag and will be removed in the future`) + this.#warnings.push([`-${s} is not a valid single-hyphen cli flag and will be removed in the future`]) } } nopt.invalidHandler = (k, val, type) => this.invalidHandler(k, val, type, 'command line options', 'cli') - nopt.unknownHandler = this.unknownHandler - nopt.abbrevHandler = this.abbrevHandler + nopt.unknownHandler = (key, next) => this.unknownHandler(key, next) + nopt.abbrevHandler = (short, long) => this.abbrevHandler(short, long) const conf = nopt(this.types, this.shorthands, this.argv) nopt.invalidHandler = null nopt.unknownHandler = null @@ -529,11 +558,11 @@ class Config { invalidHandler (k, val, type, source, where) { const typeDescription = require('./type-description.js') - log.warn( + this.#warnings.push([ 'invalid config', k + '=' + JSON.stringify(val), - `set in ${source}` - ) + `set in ${source}`, + ]) this.data.get(where)[_valid] = false if (Array.isArray(type)) { @@ -555,16 +584,16 @@ class Config { const msg = 'Must be' + this.#getOneOfKeywords(mustBe, typeDesc) const desc = mustBe.length === 1 ? mustBe[0] : [...new Set(mustBe.map(n => typeof n === 'string' ? n : JSON.stringify(n)))].join(', ') - log.warn('invalid config', msg, desc) + this.#warnings.push(['invalid config', msg, desc]) } abbrevHandler (short, long) { - log.warn(`Expanding --${short} to --${long}. This will stop working in the next major version of npm.`) + this.#warnings.push([`Expanding --${short} to --${long}. This will stop working in the next major version of npm.`]) } unknownHandler (key, next) { if (next) { - log.warn(`"${next}" is being parsed as a normal command line argument.`) + this.#warnings.push([`"${next}" is being parsed as a normal command line argument.`]) } } @@ -582,37 +611,6 @@ class Config { return keyword } - // Parse and set config entries from an object of raw config values - // Used by both #loadObject (initial load) and loadDefinitions (re-parse with new definitions) - #parseConfigEntries (obj, where, configData, filter = null) { - for (const [key, value] of Object.entries(obj)) { - // If filter is provided, only process keys that pass the filter - if (filter && !filter(key)) { - continue - } - - const k = envReplace(key, this.env) - const v = this.parseField(value, k) - - if (where !== 'default') { - this.#checkDeprecated(k) - if (this.definitions[key]?.exclusive) { - for (const exclusive of this.definitions[key].exclusive) { - if (!this.isDefault(exclusive)) { - throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) - } - } - } - } - - if (where !== 'default' || key === 'npm-version') { - this.checkUnknown(where, key, false) - } - - configData.data[k] = v - } - } - #loadObject (obj, where, source, er = null) { // obj is the raw data read from the file const conf = this.data.get(where) @@ -637,32 +635,46 @@ class Config { } } else { conf.raw = obj - this.#parseConfigEntries(obj, where, conf) + for (const [key, value] of Object.entries(obj)) { + const k = envReplace(key, this.env) + const v = this.parseField(value, k) + if (where !== 'default') { + this.#checkDeprecated(k) + if (this.definitions[key]?.exclusive) { + for (const exclusive of this.definitions[key].exclusive) { + if (!this.isDefault(exclusive)) { + throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) + } + } + } + } + if (where !== 'default' || key === 'npm-version') { + this.checkUnknown(where, key) + } + conf.data[k] = v + } } } - checkUnknown (where, key, emitWarnings = false) { - if (!emitWarnings) { - return - } + checkUnknown (where, key) { if (!this.definitions[key]) { if (internalEnv.includes(key)) { return } if (!key.includes(':')) { - log.warn(`Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.`) + this.#warnings.push([`Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.`]) return } const baseKey = key.split(':').pop() if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { - log.warn(`Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`) + this.#warnings.push([`Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`]) } } } #checkDeprecated (key) { if (this.deprecated[key]) { - log.warn('config', key, this.deprecated[key]) + this.#warnings.push(['config', key, this.deprecated[key]]) } } @@ -672,11 +684,27 @@ class Config { } async #loadFile (file, type) { + // Check cache first + if (this.#fileCache.has(file)) { + const cached = this.#fileCache.get(file) + if (cached.error) { + return this.#loadObject(null, type, file, cached.error) + } else { + return this.#loadObject(cached.data, type, file) + } + } + + if (!file) { + return + } + // only catch the error from readFile, not from the loadObject call log.silly('config', `load:file:${file}`) await readFile(file, 'utf8').then( data => { const parsedConfig = ini.parse(data) + // Cache the parsed config + this.#fileCache.set(file, { data: parsedConfig }) if (type === 'project' && parsedConfig.prefix) { // Log error if prefix is mentioned in project .npmrc /* eslint-disable-next-line max-len */ @@ -684,7 +712,11 @@ class Config { } return this.#loadObject(parsedConfig, type, file) }, - er => this.#loadObject(null, type, file, er) + er => { + // Cache the error + this.#fileCache.set(file, { error: er }) + return this.#loadObject(null, type, file, er) + } ) } @@ -701,7 +733,7 @@ class Config { // if we have not detected a local package json yet, try now that we // have a local prefix if (this.localPackage == null) { - this.localPackage = await fileExists(this.localPrefix, 'package.json') + this.localPackage = await this.#cachedFileExists(this.localPrefix, 'package.json') } if (this.#get('global') === true || this.#get('location') === 'global') { @@ -724,6 +756,36 @@ class Config { } } + async #cachedFileExists (...p) { + const key = resolve(...p) + if (this.#fileExistsCache.has(key)) { + return this.#fileExistsCache.get(key) + } + const result = await fileExists(...p) + this.#fileExistsCache.set(key, result) + return result + } + + async #cachedDirExists (...p) { + const key = resolve(...p) + if (this.#dirExistsCache.has(key)) { + return this.#dirExistsCache.get(key) + } + const result = await dirExists(...p) + this.#dirExistsCache.set(key, result) + return result + } + + async #cachedPkgJsonNormalize (p) { + if (this.#pkgJsonCache.has(p)) { + return this.#pkgJsonCache.get(p) + } + const pkgJson = require('@npmcli/package-json') + const result = await pkgJson.normalize(p).catch(() => ({ content: {} })) + this.#pkgJsonCache.set(p, result) + return result + } + async loadLocalPrefix () { const cliPrefix = this.#get('prefix', 'cli') if (cliPrefix) { @@ -741,9 +803,9 @@ class Config { break } - const hasPackageJson = await fileExists(p, 'package.json') + const hasPackageJson = await this.#cachedFileExists(p, 'package.json') - if (!this.localPrefix && (hasPackageJson || await dirExists(p, 'node_modules'))) { + if (!this.localPrefix && (hasPackageJson || await this.#cachedDirExists(p, 'node_modules'))) { this.localPrefix = p this.localPackage = hasPackageJson @@ -757,11 +819,10 @@ class Config { } if (this.localPrefix && hasPackageJson) { - const pkgJson = require('@npmcli/package-json') // if we already set localPrefix but this dir has a package.json // then we need to see if `p` is a workspace root by reading its package.json // however, if reading it fails then we should just move on - const { content: pkg } = await pkgJson.normalize(p).catch(() => ({ content: {} })) + const { content: pkg } = await this.#cachedPkgJsonNormalize(p) if (!pkg?.workspaces) { continue } @@ -771,8 +832,8 @@ class Config { for (const w of workspaces.values()) { if (w === this.localPrefix) { // see if there's a .npmrc file in the workspace, if so log a warning - if (await fileExists(this.localPrefix, '.npmrc')) { - log.warn('config', `ignoring workspace config at ${this.localPrefix}/.npmrc`) + if (await this.#cachedFileExists(this.localPrefix, '.npmrc')) { + this.#warnings.push(['config', `ignoring workspace config at ${this.localPrefix}/.npmrc`]) } // set the workspace in the default layer, which allows it to be overridden easily @@ -953,139 +1014,6 @@ class Config { return creds } - // Validate unknown configs with additional definitions - // This allows command-specific definitions to be checked against already-loaded config - warn (additionalDefinitions = {}) { - const originalDefs = this.definitions - - // Temporarily merge additional definitions for validation - this.definitions = { ...this.definitions, ...additionalDefinitions } - - try { - // Re-validate all loaded configs with merged definitions - for (const where of ['env', 'cli', 'builtin', 'user', 'project', 'global']) { - const { raw } = this.data.get(where) - if (!raw || !Object.keys(raw).length) { - continue - } - - for (const key of Object.keys(raw)) { - const k = envReplace(key, this.env) - this.checkUnknown(where, k, true) - } - } - } finally { - // Restore original definitions (don't mutate permanently) - this.definitions = originalDefs - } - } - - // Clone this Config instance with all its state - clone () { - const cloned = new Config({ - definitions: { ...this.definitions }, - shorthands: { ...this.shorthands }, - flatten: this.#flatten, - nerfDarts: [...this.nerfDarts], - npmPath: this.npmPath, - env: this.env, - argv: this.argv, - platform: this.platform, - execPath: this.execPath, - cwd: this.cwd, - excludeNpmCwd: this.excludeNpmCwd, - }) - - // Copy loaded state - cloned.#loaded = this.#loaded - cloned.globalPrefix = this.globalPrefix - cloned.localPrefix = this.localPrefix - cloned.localPackage = this.localPackage - cloned.home = this.home - cloned.parsedArgv = this.parsedArgv - - // Deep copy types, defaults, and deprecated to prevent mutation - cloned.types = { ...this.types } - cloned.defaults = { ...this.defaults } - cloned.deprecated = { ...this.deprecated } - - // Copy sources map - cloned.sources = new Map(this.sources) - - // Clone the data map by copying each ConfigData layer - for (const where of confTypes) { - const sourceData = this.data.get(where) - const clonedData = cloned.data.get(where) - - // Copy only own properties from data (not inherited from prototype chain) - for (const key of Object.keys(sourceData.data)) { - clonedData.data[key] = sourceData.data[key] - } - - // Copy raw config - Object.assign(clonedData.raw, sourceData.raw) - - // Copy source (use internal setter to bypass the check) - if (sourceData.source) { - clonedData.source = sourceData.source - } - - // Copy validation state - clonedData[_valid] = sourceData[_valid] - - // Copy load error if any - if (sourceData.loadError) { - clonedData[_loadError] = sourceData.loadError - } - } - - return cloned - } - - // Load additional definitions after config has been loaded - // This re-processes already-loaded config data with the new definitions - loadDefinitions (additionalDefinitions) { - if (!additionalDefinitions) { - return this - } - if (!this.loaded) { - throw new Error('call config.load() or config.loadSync() before loading additional definitions') - } - // Merge new definitions - this.definitions = { ...this.definitions, ...additionalDefinitions } - // Update types, defaults, deprecated maps - for (const [key, def] of Object.entries(additionalDefinitions)) { - this.defaults[key] = def.default - this.types[key] = def.type - if (def.deprecated) { - this.deprecated[key] = def.deprecated.trim().replace(/\n +/, '\n') - } - } - - // Re-process already loaded data with new definitions - for (const where of ['default', 'builtin', 'env', 'cli', 'user', 'project', 'global']) { - const configData = this.data.get(where) - const { raw, source } = configData - - // Skip if this layer wasn't loaded - if (!source || !raw || !Object.keys(raw).length) { - continue - } - - // Re-parse only keys that are in the new definitions - const filter = (key) => { - const k = envReplace(key, this.env) - return additionalDefinitions[k] || additionalDefinitions[key] - } - - this.#parseConfigEntries(raw, where, configData, filter) - } - - // Invalidate flat options since new definitions may affect flattening - this.#flatOptions = null - return this - } - // set up the environment object we have with npm_config_* environs // for all configs that are different from their default values, and // set EDITOR and HOME. From 91a6104ff0194c58e2d722093c1373bd9c48c622 Mon Sep 17 00:00:00 2001 From: reggi Date: Tue, 25 Nov 2025 16:57:24 -0500 Subject: [PATCH 03/12] check unknown factory --- lib/commands/config.js | 2 +- lib/commands/publish.js | 2 +- lib/commands/unpublish.js | 2 +- workspaces/config/lib/index.js | 36 ++++++++++++++++++++++------------ 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/lib/commands/config.js b/lib/commands/config.js index 30b3e6ae5690f..b657029b2d2fe 100644 --- a/lib/commands/config.js +++ b/lib/commands/config.js @@ -367,7 +367,7 @@ ${defData} if (content.publishConfig) { for (const key in content.publishConfig) { - this.npm.config.checkUnknown('publishConfig', key, true) + this.npm.config.checkUnknown('publishConfig', key) } const pkgPath = resolve(this.npm.prefix, 'package.json') msg.push(`; "publishConfig" from ${pkgPath}`) diff --git a/lib/commands/publish.js b/lib/commands/publish.js index 406b8cd91b942..162e3d65ba5ce 100644 --- a/lib/commands/publish.js +++ b/lib/commands/publish.js @@ -272,7 +272,7 @@ class Publish extends BaseCommand { Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags))) if (logWarnings) { for (const key in filteredPublishConfig) { - this.npm.config.checkUnknown('publishConfig', key, true) + this.npm.config.checkUnknown('publishConfig', key) } } flatten(filteredPublishConfig, opts) diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index b32818dc75838..e1c06d3184057 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -146,7 +146,7 @@ class Unpublish extends BaseCommand { const filteredPublishConfig = Object.fromEntries( Object.entries(manifest.publishConfig).filter(([key]) => !(key in cliFlags))) for (const key in filteredPublishConfig) { - this.npm.config.checkUnknown('publishConfig', key, true) + this.npm.config.checkUnknown('publishConfig', key) } flatten(filteredPublishConfig, opts) } diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index e21ce2364bda5..2cf731bbaa102 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -649,29 +649,39 @@ class Config { } } if (where !== 'default' || key === 'npm-version') { - this.checkUnknown(where, key) + this.#checkUnknown(where, key) } conf.data[k] = v } } } - checkUnknown (where, key) { - if (!this.definitions[key]) { - if (internalEnv.includes(key)) { - return - } - if (!key.includes(':')) { - this.#warnings.push([`Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.`]) - return - } - const baseKey = key.split(':').pop() - if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { - this.#warnings.push([`Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`]) + #checkUnknownFactory (warnFn) { + return (where, key) => { + if (!this.definitions[key]) { + if (internalEnv.includes(key)) { + return + } + if (!key.includes(':')) { + warnFn([`Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.`]) + return + } + const baseKey = key.split(':').pop() + if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { + warnFn([`Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`]) + } } } } + #checkUnknown (where, key) { + return this.#checkUnknownFactory((warning) => this.#warnings.push(warning))(where, key) + } + + checkUnknown (where, key) { + return this.#checkUnknownFactory((warning) => log.warn(...warning))(where, key) + } + #checkDeprecated (key) { if (this.deprecated[key]) { this.#warnings.push(['config', key, this.deprecated[key]]) From f9d161ff22f8ba5acb2122afc010d8ba10d69f39 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:29:03 -0500 Subject: [PATCH 04/12] 3rd attempt --- lib/base-cmd.js | 22 +- lib/npm.js | 22 +- .../test/lib/commands/install.js.test.cjs | 18 +- test/lib/npm.js | 67 +++ workspaces/config/lib/index.js | 463 ++++++++---------- 5 files changed, 308 insertions(+), 284 deletions(-) diff --git a/lib/base-cmd.js b/lib/base-cmd.js index 69a86ab14ee8d..5bacf5df5d0df 100644 --- a/lib/base-cmd.js +++ b/lib/base-cmd.js @@ -22,11 +22,13 @@ class BaseCommand { const { description, usage = [''], name, params } = this if (this.definitions) { - const cmdDefinitions = this.definitions - this.definitions = { ...globalDefinitions, ...cmdDefinitions } + this.definitions = this.definitions } else { this.definitions = globalDefinitions } + + const definitionsPool = { ...globalDefinitions, ...this.definitions } + const fullUsage = [ `${description}`, '', @@ -42,14 +44,14 @@ class BaseCommand { if (seenExclusive.has(param)) { continue } - const exclusive = this.definitions[param]?.exclusive - let paramUsage = this.definitions[param]?.usage || '' + const exclusive = definitionsPool[param]?.exclusive + let paramUsage = definitionsPool[param]?.usage if (exclusive) { const exclusiveParams = [paramUsage] seenExclusive.add(param) for (const e of exclusive) { seenExclusive.add(e) - exclusiveParams.push(this.definitions[e].usage) + exclusiveParams.push(definitionsPool[e].usage) } paramUsage = `${exclusiveParams.join('|')}` } @@ -97,21 +99,13 @@ class BaseCommand { get config () { // Return command-specific config if it exists, otherwise use npm's config - return this._config || this.npm.config - } - - set config (value) { - this._config = value + return this.npm.config } get name () { return this.constructor.name } - get definitions () { - return this.constructor.definitions - } - get description () { return this.constructor.description } diff --git a/lib/npm.js b/lib/npm.js index f1166efca8c78..5a2d3b55ad73c 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -68,13 +68,9 @@ class Npm { this.#npmRoot = npmRoot this.#argv = argv this.#excludeNpmCwd = excludeNpmCwd - this.config = this.createConfig(globalDefinitions) - } - - createConfig (definitions) { - return new Config({ + this.config = new Config({ npmPath: this.#npmRoot, - definitions, + definitions: globalDefinitions, flatten, nerfDarts, shorthands, @@ -235,16 +231,12 @@ class Npm { process.env.npm_command = this.command } - if (Command.definitions === globalDefinitions) { - this.config.warn() + if (!Command.definitions || Command.definitions === globalDefinitions) { + this.config.logWarnings() } else { - const _config = this.config.clone({ - definitions: { ...globalDefinitions, ...Command.definitions }, - shouldSetEnvs: true, - }) - await _config.load() - _config.warn() - command._config = _config + this.config.loadCommand(Command.definitions) + this.config.logWarnings() + this.config.warn = true } if (this.config.get('usage')) { diff --git a/tap-snapshots/test/lib/commands/install.js.test.cjs b/tap-snapshots/test/lib/commands/install.js.test.cjs index 02df3c178fd83..20ae362cdce0e 100644 --- a/tap-snapshots/test/lib/commands/install.js.test.cjs +++ b/tap-snapshots/test/lib/commands/install.js.test.cjs @@ -134,9 +134,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -199,9 +199,9 @@ warn EBADDEVENGINES } verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -225,9 +225,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:205:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:267:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:216:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime diff --git a/test/lib/npm.js b/test/lib/npm.js index b4ac509adb495..b12f0ab780786 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -5,6 +5,7 @@ const { time } = require('proc-log') const { load: loadMockNpm } = require('../fixtures/mock-npm.js') const mockGlobals = require('@npmcli/mock-globals') const { commands } = require('../../lib/utils/cmd-list.js') +const BaseCommand = require('../../lib/base-cmd.js') t.test('not yet loaded', async t => { const { npm, logs } = await loadMockNpm(t, { load: false }) @@ -567,3 +568,69 @@ t.test('print usage if non-command param provided', async t => { t.match(joinedOutput(), 'Unknown command: "tset"') t.match(joinedOutput(), 'Did you mean this?') }) + +async function testCommandDefinitions (t, { defaultValue, outputValue, type, flags }) { + const path = require('node:path') + + // Create a temporary command file + const tsetPath = path.join(__dirname, '../../lib/commands/tset.js') + const tsetContent = ` +const Definition = require('@npmcli/config/lib/definitions/definition.js') +const BaseCommand = require('../base-cmd.js') +const { output } = require('proc-log') +const { flatten } = require('@npmcli/config/lib/definitions/index.js') + +module.exports = class TestCommand extends BaseCommand { + static description = 'A test command' + static name = 'tset' + static definitions = { + say: new Definition('say', { + default: ${defaultValue}, + type: ${type}, + description: 'say', + flatten, + }), + } + + async exec () { + const say = this.npm.config.get('say') + output.standard(say) + } +} +` + fs.writeFileSync(tsetPath, tsetContent) + t.teardown(() => { + try { + fs.unlinkSync(tsetPath) + delete require.cache[tsetPath] + } catch (e) { + // ignore + } + }) + + const mockCmdList = require('../../lib/utils/cmd-list.js') + const { npm, joinedOutput } = await loadMockNpm(t, { + mocks: { + '{LIB}/utils/cmd-list.js': { + ...mockCmdList, + commands: [...mockCmdList.commands, 'tset'], + deref: (c) => c === 'tset' ? 'tset' : mockCmdList.deref(c), + }, + }, + }) + + // Now you can execute the mocked command + await npm.exec('tset', flags || []) + + t.match(joinedOutput(), outputValue) +} + +const stack = { + boolean_default: (t) => testCommandDefinitions(t, { type: 'Boolean', defaultValue: 'false', outputValue: 'false' }), + string_default: (t) => testCommandDefinitions(t, { type: 'String', defaultValue: `'meow'`, outputValue: 'meow' }), + string_flag: (t) => testCommandDefinitions(t, { type: 'String', defaultValue: `'meow'`, outputValue: 'woof', flags: ['--say=woof'] }), +} + +Object.entries(stack).forEach(([name, fn]) => { + t.test(name, fn) +}) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 2cf731bbaa102..4bc06dc077382 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -1,4 +1,5 @@ // TODO: set the scope config from package.json or explicit cli config +const { definitions: globalDefinitions } = require('./definitions') const { walkUp } = require('walk-up-path') const ini = require('ini') const nopt = require('nopt') @@ -51,6 +52,7 @@ const confTypes = new Set([ 'builtin', ...confFileTypes, 'env', + 'flags', 'cli', ]) @@ -59,43 +61,13 @@ class Config { #flatten // populated the first time we flatten the object #flatOptions = null - // cache for filesystem operations to allow efficient reloading - #fileCache = new Map() - #fileExistsCache = new Map() - #dirExistsCache = new Map() - #pkgJsonCache = new Map() #warnings = [] static get typeDefs () { return typeDefs } - clone (args) { - return new Config({ - cache: { - fileCache: this.#fileCache, - fileExistsCache: this.#fileExistsCache, - dirExistsCache: this.#dirExistsCache, - pkgJsonCache: this.#pkgJsonCache, - }, - definitions: this.definitions, - shorthands: this.shorthands, - flatten: this.#flatten, - nerfDarts: this.nerfDarts, - npmPath: this.npmPath, - env: this.env, - argv: this.argv, - platform: this.platform, - execPath: this.execPath, - cwd: this.cwd, - excludeNpmCwd: this.excludeNpmCwd, - shouldSetEnvs: this.shouldSetEnvs, - ...args, - }) - } - constructor ({ - cache, definitions, shorthands, flatten, @@ -109,30 +81,13 @@ class Config { execPath = process.execPath, cwd = process.cwd(), excludeNpmCwd = false, - shouldSetEnvs = false, }) { - if (cache) { - this.#fileCache = cache.fileCache - this.#fileExistsCache = cache.fileExistsCache - this.#dirExistsCache = cache.dirExistsCache - this.#pkgJsonCache = cache.pkgJsonCache - } - this.#warnings = [] - this.shouldSetEnvs = shouldSetEnvs this.nerfDarts = nerfDarts this.definitions = definitions // turn the definitions into nopt's weirdo syntax - const types = {} - const defaults = {} - this.deprecated = {} - for (const [key, def] of Object.entries(definitions)) { - defaults[key] = def.default - types[key] = def.type - if (def.deprecated) { - this.deprecated[key] = def.deprecated.trim().replace(/\n +/, '\n') - } - } + const { types, defaults, deprecated } = this.getTypesFromDefinitions(definitions) + this.deprecated = deprecated this.#flatten = flatten this.types = types this.shorthands = shorthands @@ -177,6 +132,144 @@ class Config { } this.#loaded = false + + this.warn = true + + this.log = { + warn: (type, ...args) => { + if (!this.warn) { + this.#warnings.push({ type, args }) + } else { + log.warn(...args) + } + }, + } + } + + // Private methods - must be declared before use + #get (key, where = null) { + if (where !== null && !confTypes.has(where)) { + throw new Error('invalid config location param: ' + where) + } + const { data } = this.data.get(where || 'cli') + return where === null || hasOwnProperty(data, key) ? data[key] : undefined + } + + #checkDeprecated (key) { + if (this.deprecated[key]) { + this.log.warn(`deprecated:${key}`, 'config', key, this.deprecated[key]) + } + } + + #getFlags (types) { + for (const s of Object.keys(this.shorthands)) { + if (s.length > 1 && this.argv.includes(`-${s}`)) { + log.warn(`-${s} is not a valid single-hyphen cli flag and will be removed in the future`) + } + } + nopt.invalidHandler = (k, val, type) => + this.invalidHandler(k, val, type, 'command line options', 'cli') + nopt.unknownHandler = this.unknownHandler + nopt.abbrevHandler = this.abbrevHandler + const conf = nopt(types, this.shorthands, this.argv) + nopt.invalidHandler = null + nopt.unknownHandler = null + this.parsedArgv = conf.argv + delete conf.argv + return conf + } + + #getOneOfKeywords (mustBe, typeDesc) { + let keyword + if (mustBe.length === 1 && typeDesc.includes(Array)) { + keyword = ' one or more' + } else if (mustBe.length > 1 && typeDesc.includes(Array)) { + keyword = ' one or more of:' + } else if (mustBe.length > 1) { + keyword = ' one of:' + } else { + keyword = '' + } + return keyword + } + + #loadObject (obj, where, source, er = null) { + // obj is the raw data read from the file + const conf = this.data.get(where) + if (conf.source) { + const m = `double-loading "${where}" configs from ${source}, ` + + `previously loaded from ${conf.source}` + throw new Error(m) + } + + if (this.sources.has(source)) { + const m = `double-loading config "${source}" as "${where}", ` + + `previously loaded as "${this.sources.get(source)}"` + throw new Error(m) + } + + conf.source = source + this.sources.set(source, where) + if (er) { + conf.loadError = er + if (er.code !== 'ENOENT') { + log.verbose('config', `error loading ${where} config`, er) + } + } else { + conf.raw = obj + for (const [key, value] of Object.entries(obj)) { + const k = envReplace(key, this.env) + const v = this.parseField(value, k) + if (where !== 'default') { + this.#checkDeprecated(k) + if (this.definitions[key]?.exclusive) { + for (const exclusive of this.definitions[key].exclusive) { + if (!this.isDefault(exclusive)) { + throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) + } + } + } + } + if (where !== 'default' || key === 'npm-version') { + this.checkUnknown(where, key) + } + conf.data[k] = v + } + } + } + + async #loadFile (file, type) { + // only catch the error from readFile, not from the loadObject call + log.silly('config', `load:file:${file}`) + await readFile(file, 'utf8').then( + data => { + const parsedConfig = ini.parse(data) + if (type === 'project' && parsedConfig.prefix) { + // Log error if prefix is mentioned in project .npmrc + /* eslint-disable-next-line max-len */ + log.error('config', `prefix cannot be changed from project config: ${file}.`) + } + return this.#loadObject(parsedConfig, type, file) + }, + er => this.#loadObject(null, type, file, er) + ) + } + + getTypesFromDefinitions (definitions) { + if (!definitions) { + definitions = {} + } + const types = {} + const defaults = {} + const deprecated = {} + for (const [key, def] of Object.entries(definitions)) { + defaults[key] = def.default + types[key] = def.type + if (def.deprecated) { + deprecated[key] = def.deprecated.trim().replace(/\n +/, '\n') + } + } + return { types, defaults, deprecated } } get list () { @@ -191,16 +284,33 @@ class Config { return this.#loaded } - warn () { - for (const warning of this.#warnings) { - log.warn(...warning) - } - } - get prefix () { return this.#get('global') ? this.globalPrefix : this.localPrefix } + removeWarnings (types) { + const typeSet = new Set(Array.isArray(types) ? types : [types]) + this.#warnings = this.#warnings.filter(w => !typeSet.has(w.type)) + } + + #deduplicateWarnings () { + const seen = new Set() + this.#warnings = this.#warnings.filter(w => { + if (seen.has(w.type)) { + return false + } + seen.add(w.type) + return true + }) + } + + logWarnings () { + for (const warning of this.#warnings) { + log.warn(...warning.args) + } + this.#warnings = [] + } + // return the location where key is found. find (key) { if (!this.loaded) { @@ -225,16 +335,6 @@ class Config { return this.#get(key, where) } - // we need to get values sometimes, so use this internal one to do so - // while in the process of loading. - #get (key, where = null) { - if (where !== null && !confTypes.has(where)) { - throw new Error('invalid config location param: ' + where) - } - const { data } = this.data.get(where || 'cli') - return where === null || hasOwnProperty(data, key) ? data[key] : undefined - } - set (key, val, where = 'cli') { if (!this.loaded) { throw new Error('call config.load() before setting values') @@ -320,9 +420,7 @@ class Config { // set proper globalPrefix now that everything is loaded this.globalPrefix = this.get('prefix') - if (this.shouldSetEnvs) { - setEnvs(this) - } + this.setEnvs() } loadDefaults () { @@ -410,23 +508,36 @@ class Config { } loadCLI () { - for (const s of Object.keys(this.shorthands)) { - if (s.length > 1 && this.argv.includes(`-${s}`)) { - this.#warnings.push([`-${s} is not a valid single-hyphen cli flag and will be removed in the future`]) - } - } - nopt.invalidHandler = (k, val, type) => - this.invalidHandler(k, val, type, 'command line options', 'cli') - nopt.unknownHandler = (key, next) => this.unknownHandler(key, next) - nopt.abbrevHandler = (short, long) => this.abbrevHandler(short, long) - const conf = nopt(this.types, this.shorthands, this.argv) - nopt.invalidHandler = null - nopt.unknownHandler = null - this.parsedArgv = conf.argv - delete conf.argv + const conf = this.#getFlags(this.types) this.#loadObject(conf, 'cli', 'command line options') } + loadCommand (definitions) { + // Merge command definitions with global definitions + this.definitions = { ...this.definitions, ...definitions } + const { defaults, types, deprecated } = this.getTypesFromDefinitions(definitions) + this.deprecated = { ...this.deprecated, ...deprecated } + this.types = { ...this.types, ...types } + + // Re-parse with merged definitions + const conf = this.#getFlags(this.types) + + // Remove warnings for keys that are now defined + const keysToRemove = Object.keys(definitions).flatMap(key => [ + `unknown:${key}`, + `deprecated:${key}`, + ]) + this.removeWarnings(keysToRemove) + + console.log({ defaults, conf }) + + // Load into new command source - only command-specific defaults + parsed flags + this.#loadObject({ ...defaults, ...conf }, 'flags', 'command-specific flag options') + + // Deduplicate warnings by type (e.g., unknown:key warnings from both cli and flags) + this.#deduplicateWarnings() + } + get valid () { for (const [where, { valid }] of this.data.entries()) { if (valid === false || valid === null && !this.validate(where)) { @@ -558,11 +669,12 @@ class Config { invalidHandler (k, val, type, source, where) { const typeDescription = require('./type-description.js') - this.#warnings.push([ + this.log.warn( + 'invalid', 'invalid config', k + '=' + JSON.stringify(val), - `set in ${source}`, - ]) + `set in ${source}` + ) this.data.get(where)[_valid] = false if (Array.isArray(type)) { @@ -584,152 +696,40 @@ class Config { const msg = 'Must be' + this.#getOneOfKeywords(mustBe, typeDesc) const desc = mustBe.length === 1 ? mustBe[0] : [...new Set(mustBe.map(n => typeof n === 'string' ? n : JSON.stringify(n)))].join(', ') - this.#warnings.push(['invalid config', msg, desc]) + this.log.warn('invalid', 'invalid config', msg, desc) } abbrevHandler (short, long) { - this.#warnings.push([`Expanding --${short} to --${long}. This will stop working in the next major version of npm.`]) + log.warn(`Expanding --${short} to --${long}. This will stop working in the next major version of npm.`) } unknownHandler (key, next) { if (next) { - this.#warnings.push([`"${next}" is being parsed as a normal command line argument.`]) - } - } - - #getOneOfKeywords (mustBe, typeDesc) { - let keyword - if (mustBe.length === 1 && typeDesc.includes(Array)) { - keyword = ' one or more' - } else if (mustBe.length > 1 && typeDesc.includes(Array)) { - keyword = ' one or more of:' - } else if (mustBe.length > 1) { - keyword = ' one of:' - } else { - keyword = '' + log.warn(`"${next}" is being parsed as a normal command line argument.`) } - return keyword } - #loadObject (obj, where, source, er = null) { - // obj is the raw data read from the file - const conf = this.data.get(where) - if (conf.source) { - const m = `double-loading "${where}" configs from ${source}, ` + - `previously loaded from ${conf.source}` - throw new Error(m) - } - - if (this.sources.has(source)) { - const m = `double-loading config "${source}" as "${where}", ` + - `previously loaded as "${this.sources.get(source)}"` - throw new Error(m) - } - - conf.source = source - this.sources.set(source, where) - if (er) { - conf.loadError = er - if (er.code !== 'ENOENT') { - log.verbose('config', `error loading ${where} config`, er) + checkUnknown (where, key) { + if (!this.definitions[key]) { + if (internalEnv.includes(key)) { + return } - } else { - conf.raw = obj - for (const [key, value] of Object.entries(obj)) { - const k = envReplace(key, this.env) - const v = this.parseField(value, k) - if (where !== 'default') { - this.#checkDeprecated(k) - if (this.definitions[key]?.exclusive) { - for (const exclusive of this.definitions[key].exclusive) { - if (!this.isDefault(exclusive)) { - throw new TypeError(`--${key} cannot be provided when using --${exclusive}`) - } - } - } - } - if (where !== 'default' || key === 'npm-version') { - this.#checkUnknown(where, key) - } - conf.data[k] = v + if (!key.includes(':')) { + this.log.warn(`unknown:${key}`, `Unknown ${where} config "${where === 'cli' || where === 'flags' ? '--' : ''}${key}". This will stop working in the next major version of npm.`) + return } - } - } - - #checkUnknownFactory (warnFn) { - return (where, key) => { - if (!this.definitions[key]) { - if (internalEnv.includes(key)) { - return - } - if (!key.includes(':')) { - warnFn([`Unknown ${where} config "${where === 'cli' ? '--' : ''}${key}". This will stop working in the next major version of npm.`]) - return - } - const baseKey = key.split(':').pop() - if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { - warnFn([`Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`]) - } + const baseKey = key.split(':').pop() + if (!this.definitions[baseKey] && !this.nerfDarts.includes(baseKey)) { + this.log.warn(`unknown:${baseKey}`, `Unknown ${where} config "${baseKey}" (${key}). This will stop working in the next major version of npm.`) } } } - #checkUnknown (where, key) { - return this.#checkUnknownFactory((warning) => this.#warnings.push(warning))(where, key) - } - - checkUnknown (where, key) { - return this.#checkUnknownFactory((warning) => log.warn(...warning))(where, key) - } - - #checkDeprecated (key) { - if (this.deprecated[key]) { - this.#warnings.push(['config', key, this.deprecated[key]]) - } - } - // Parse a field, coercing it to the best type available. parseField (f, key, listElement = false) { return parseField(f, key, this, listElement) } - async #loadFile (file, type) { - // Check cache first - if (this.#fileCache.has(file)) { - const cached = this.#fileCache.get(file) - if (cached.error) { - return this.#loadObject(null, type, file, cached.error) - } else { - return this.#loadObject(cached.data, type, file) - } - } - - if (!file) { - return - } - - // only catch the error from readFile, not from the loadObject call - log.silly('config', `load:file:${file}`) - await readFile(file, 'utf8').then( - data => { - const parsedConfig = ini.parse(data) - // Cache the parsed config - this.#fileCache.set(file, { data: parsedConfig }) - if (type === 'project' && parsedConfig.prefix) { - // Log error if prefix is mentioned in project .npmrc - /* eslint-disable-next-line max-len */ - log.error('config', `prefix cannot be changed from project config: ${file}.`) - } - return this.#loadObject(parsedConfig, type, file) - }, - er => { - // Cache the error - this.#fileCache.set(file, { error: er }) - return this.#loadObject(null, type, file, er) - } - ) - } - loadBuiltinConfig () { return this.#loadFile(resolve(this.npmPath, 'npmrc'), 'builtin') } @@ -743,7 +743,7 @@ class Config { // if we have not detected a local package json yet, try now that we // have a local prefix if (this.localPackage == null) { - this.localPackage = await this.#cachedFileExists(this.localPrefix, 'package.json') + this.localPackage = await fileExists(this.localPrefix, 'package.json') } if (this.#get('global') === true || this.#get('location') === 'global') { @@ -766,36 +766,6 @@ class Config { } } - async #cachedFileExists (...p) { - const key = resolve(...p) - if (this.#fileExistsCache.has(key)) { - return this.#fileExistsCache.get(key) - } - const result = await fileExists(...p) - this.#fileExistsCache.set(key, result) - return result - } - - async #cachedDirExists (...p) { - const key = resolve(...p) - if (this.#dirExistsCache.has(key)) { - return this.#dirExistsCache.get(key) - } - const result = await dirExists(...p) - this.#dirExistsCache.set(key, result) - return result - } - - async #cachedPkgJsonNormalize (p) { - if (this.#pkgJsonCache.has(p)) { - return this.#pkgJsonCache.get(p) - } - const pkgJson = require('@npmcli/package-json') - const result = await pkgJson.normalize(p).catch(() => ({ content: {} })) - this.#pkgJsonCache.set(p, result) - return result - } - async loadLocalPrefix () { const cliPrefix = this.#get('prefix', 'cli') if (cliPrefix) { @@ -813,9 +783,9 @@ class Config { break } - const hasPackageJson = await this.#cachedFileExists(p, 'package.json') + const hasPackageJson = await fileExists(p, 'package.json') - if (!this.localPrefix && (hasPackageJson || await this.#cachedDirExists(p, 'node_modules'))) { + if (!this.localPrefix && (hasPackageJson || await dirExists(p, 'node_modules'))) { this.localPrefix = p this.localPackage = hasPackageJson @@ -829,10 +799,11 @@ class Config { } if (this.localPrefix && hasPackageJson) { + const pkgJson = require('@npmcli/package-json') // if we already set localPrefix but this dir has a package.json // then we need to see if `p` is a workspace root by reading its package.json // however, if reading it fails then we should just move on - const { content: pkg } = await this.#cachedPkgJsonNormalize(p) + const { content: pkg } = await pkgJson.normalize(p).catch(() => ({ content: {} })) if (!pkg?.workspaces) { continue } @@ -842,8 +813,8 @@ class Config { for (const w of workspaces.values()) { if (w === this.localPrefix) { // see if there's a .npmrc file in the workspace, if so log a warning - if (await this.#cachedFileExists(this.localPrefix, '.npmrc')) { - this.#warnings.push(['config', `ignoring workspace config at ${this.localPrefix}/.npmrc`]) + if (await fileExists(this.localPrefix, '.npmrc')) { + log.warn('config', `ignoring workspace config at ${this.localPrefix}/.npmrc`) } // set the workspace in the default layer, which allows it to be overridden easily From de87bd2771115831115870d740a524f9ff9652dc Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:31:08 -0500 Subject: [PATCH 05/12] cleanup --- lib/npm.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/npm.js b/lib/npm.js index 5a2d3b55ad73c..abf1ceb9fb562 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -66,16 +66,14 @@ class Npm { } = {}) { this.#display = new Display({ stdout, stderr }) this.#npmRoot = npmRoot - this.#argv = argv - this.#excludeNpmCwd = excludeNpmCwd this.config = new Config({ npmPath: this.#npmRoot, definitions: globalDefinitions, flatten, nerfDarts, shorthands, - argv: [...process.argv, ...this.#argv], - excludeNpmCwd: this.#excludeNpmCwd, + argv: [...process.argv, ...argv], + excludeNpmCwd, }) } @@ -335,7 +333,7 @@ class Npm { ...opts, }) - const { writeFileSync, glob } = require('node:fs') + const { writeFileSync } = require('node:fs') for (const [file, content] of files) { const filePath = `${this.logPath}${file}` const fileContent = `'Log files:\n${this.logFiles.join('\n')}\n\n${content.trim()}\n` From 3d32f7e229d2e3ff4a6df9783ea70b9e635f5922 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:32:21 -0500 Subject: [PATCH 06/12] cleanup --- workspaces/config/lib/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 4bc06dc077382..a1c99cb261ed9 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -1,5 +1,4 @@ // TODO: set the scope config from package.json or explicit cli config -const { definitions: globalDefinitions } = require('./definitions') const { walkUp } = require('walk-up-path') const ini = require('ini') const nopt = require('nopt') From 32969c3d4cd68372cb6e9d648655ec227581bd63 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:33:39 -0500 Subject: [PATCH 07/12] lintfix --- test/lib/npm.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/lib/npm.js b/test/lib/npm.js index b12f0ab780786..01df48a0ae574 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -5,7 +5,6 @@ const { time } = require('proc-log') const { load: loadMockNpm } = require('../fixtures/mock-npm.js') const mockGlobals = require('@npmcli/mock-globals') const { commands } = require('../../lib/utils/cmd-list.js') -const BaseCommand = require('../../lib/base-cmd.js') t.test('not yet loaded', async t => { const { npm, logs } = await loadMockNpm(t, { load: false }) From 1d62714d83f7f94b8a1796a1a31fbedc313bab83 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:45:12 -0500 Subject: [PATCH 08/12] small fixes --- .../test/lib/commands/install.js.test.cjs | 18 ++++++++-------- test/lib/npm.js | 3 ++- workspaces/config/lib/index.js | 21 +++++++++---------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/tap-snapshots/test/lib/commands/install.js.test.cjs b/tap-snapshots/test/lib/commands/install.js.test.cjs index 20ae362cdce0e..d5315397aaf4e 100644 --- a/tap-snapshots/test/lib/commands/install.js.test.cjs +++ b/tap-snapshots/test/lib/commands/install.js.test.cjs @@ -134,9 +134,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:195:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:262:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:210:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -199,9 +199,9 @@ warn EBADDEVENGINES } verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:195:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:262:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:210:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime @@ -225,9 +225,9 @@ silly logfile done cleaning log files verbose stack Error: The developer of this package has specified the following through devEngines verbose stack Invalid devEngines.runtime verbose stack Invalid name "nondescript" does not match "node" for "runtime" -verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:200:27) -verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:264:7) -verbose stack at MockNpm.exec ({CWD}/lib/npm.js:212:9) +verbose stack at Install.checkDevEngines ({CWD}/lib/base-cmd.js:195:27) +verbose stack at MockNpm.#exec ({CWD}/lib/npm.js:262:7) +verbose stack at MockNpm.exec ({CWD}/lib/npm.js:210:9) error code EBADDEVENGINES error EBADDEVENGINES The developer of this package has specified the following through devEngines error EBADDEVENGINES Invalid devEngines.runtime diff --git a/test/lib/npm.js b/test/lib/npm.js index 01df48a0ae574..098b1fc193906 100644 --- a/test/lib/npm.js +++ b/test/lib/npm.js @@ -609,6 +609,7 @@ module.exports = class TestCommand extends BaseCommand { const mockCmdList = require('../../lib/utils/cmd-list.js') const { npm, joinedOutput } = await loadMockNpm(t, { + argv: ['tset', ...(flags || [])], mocks: { '{LIB}/utils/cmd-list.js': { ...mockCmdList, @@ -619,7 +620,7 @@ module.exports = class TestCommand extends BaseCommand { }) // Now you can execute the mocked command - await npm.exec('tset', flags || []) + await npm.exec('tset', []) t.match(joinedOutput(), outputValue) } diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index a1c99cb261ed9..443c76e202e6c 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -145,15 +145,6 @@ class Config { } } - // Private methods - must be declared before use - #get (key, where = null) { - if (where !== null && !confTypes.has(where)) { - throw new Error('invalid config location param: ' + where) - } - const { data } = this.data.get(where || 'cli') - return where === null || hasOwnProperty(data, key) ? data[key] : undefined - } - #checkDeprecated (key) { if (this.deprecated[key]) { this.log.warn(`deprecated:${key}`, 'config', key, this.deprecated[key]) @@ -327,6 +318,16 @@ class Config { return null } + // we need to get values sometimes, so use this internal one to do so + // while in the process of loading. + #get (key, where = null) { + if (where !== null && !confTypes.has(where)) { + throw new Error('invalid config location param: ' + where) + } + const { data } = this.data.get(where || 'cli') + return where === null || hasOwnProperty(data, key) ? data[key] : undefined + } + get (key, where) { if (!this.loaded) { throw new Error('call config.load() before reading values') @@ -528,8 +529,6 @@ class Config { ]) this.removeWarnings(keysToRemove) - console.log({ defaults, conf }) - // Load into new command source - only command-specific defaults + parsed flags this.#loadObject({ ...defaults, ...conf }, 'flags', 'command-specific flag options') From 989f5f4946b109d6db3a1d4c3606507a121e6ac3 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:46:02 -0500 Subject: [PATCH 09/12] less --- lib/base-cmd.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/base-cmd.js b/lib/base-cmd.js index 5bacf5df5d0df..b26cbb8c4e284 100644 --- a/lib/base-cmd.js +++ b/lib/base-cmd.js @@ -1,5 +1,5 @@ const { log } = require('proc-log') -const { definitions: globalDefinitions } = require('@npmcli/config/lib/definitions') +const { definitions } = require('@npmcli/config/lib/definitions') class BaseCommand { // these defaults can be overridden by individual commands @@ -24,10 +24,10 @@ class BaseCommand { if (this.definitions) { this.definitions = this.definitions } else { - this.definitions = globalDefinitions + this.definitions = definitions } - const definitionsPool = { ...globalDefinitions, ...this.definitions } + const definitionsPool = { ...definitions, ...this.definitions } const fullUsage = [ `${description}`, From 0dc72baca14210a3f0b074b624c1b3f304e24af6 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 19:46:53 -0500 Subject: [PATCH 10/12] definitions --- lib/npm.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/npm.js b/lib/npm.js index abf1ceb9fb562..82abaec752361 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -2,7 +2,7 @@ const { resolve, dirname, join } = require('node:path') const Config = require('@npmcli/config') const which = require('which') const fs = require('node:fs/promises') -const { definitions: globalDefinitions, flatten, nerfDarts, shorthands } = require('@npmcli/config/lib/definitions') +const { definitions, flatten, nerfDarts, shorthands } = require('@npmcli/config/lib/definitions') const usage = require('./utils/npm-usage.js') const LogFile = require('./utils/log-file.js') const Timers = require('./utils/timers.js') @@ -68,7 +68,7 @@ class Npm { this.#npmRoot = npmRoot this.config = new Config({ npmPath: this.#npmRoot, - definitions: globalDefinitions, + definitions, flatten, nerfDarts, shorthands, @@ -229,7 +229,7 @@ class Npm { process.env.npm_command = this.command } - if (!Command.definitions || Command.definitions === globalDefinitions) { + if (!Command.definitions || Command.definitions === definitions) { this.config.logWarnings() } else { this.config.loadCommand(Command.definitions) From 69b43c347ab8a523addf1166a3d3438951aff202 Mon Sep 17 00:00:00 2001 From: reggi Date: Wed, 26 Nov 2025 20:02:48 -0500 Subject: [PATCH 11/12] config coverage --- workspaces/config/test/index.js | 270 ++++++++++++++++++++++++++++++++ 1 file changed, 270 insertions(+) diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index f60070d419bfd..17e4f35b34222 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -1587,3 +1587,273 @@ t.test('abbreviation expansion warnings', async t => { ['warn', 'Expanding --bef to --before. This will stop working in the next major version of npm'], ], 'Warns about expanded abbreviations') }) + +t.test('warning suppression and logging', async t => { + const path = t.testdir() + const logs = [] + const logHandler = (...args) => logs.push(args) + process.on('log', logHandler) + t.teardown(() => process.off('log', logHandler)) + + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--unknown-key', 'value'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + // Load first to collect warnings + await config.load() + + // Now disable warnings and trigger more + config.warn = false + config.log.warn('test-type', 'test warning 1') + config.log.warn('test-type2', 'test warning 2') + + // Should have warnings collected but not logged + const initialWarnings = logs.filter(l => l[0] === 'warn') + const beforeCount = initialWarnings.length + + // Now log the warnings + config.warn = true + config.logWarnings() + const afterLogging = logs.filter(l => l[0] === 'warn') + t.ok(afterLogging.length > beforeCount, 'warnings logged after logWarnings()') + + // Calling logWarnings again should not add more warnings + const warningCount = afterLogging.length + config.logWarnings() + const finalWarnings = logs.filter(l => l[0] === 'warn') + t.equal(finalWarnings.length, warningCount, 'no duplicate warnings after second logWarnings()') +}) + +t.test('removeWarnings', async t => { + const path = t.testdir() + const logs = [] + const logHandler = (...args) => logs.push(args) + process.on('log', logHandler) + t.teardown(() => process.off('log', logHandler)) + + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--unknown1', 'value', '--unknown2', 'value'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + config.warn = false + await config.load() + + // Remove specific warning types + config.removeWarnings('unknown:unknown1') + config.logWarnings() + + const warnings = logs.filter(l => l[0] === 'warn') + const hasUnknown1 = warnings.some(w => w[1].includes('unknown1')) + const hasUnknown2 = warnings.some(w => w[1].includes('unknown2')) + + t.notOk(hasUnknown1, 'unknown1 warning removed') + t.ok(hasUnknown2, 'unknown2 warning still present') +}) + +t.test('removeWarnings with array', async t => { + const path = t.testdir() + const logs = [] + const logHandler = (...args) => logs.push(args) + process.on('log', logHandler) + t.teardown(() => process.off('log', logHandler)) + + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--unknown1', 'value', '--unknown2', 'value'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + config.warn = false + await config.load() + + // Count warnings before removal + const beforeRemoval = logs.filter(l => l[0] === 'warn').length + + // Remove multiple warning types + config.removeWarnings(['unknown:unknown1', 'unknown:unknown2']) + config.logWarnings() + + const warnings = logs.filter(l => l[0] === 'warn') + // Check that no new unknown1 or unknown2 warnings were added + const hasUnknown1 = warnings.slice(beforeRemoval).some(w => w[1].includes('unknown1')) + const hasUnknown2 = warnings.slice(beforeRemoval).some(w => w[1].includes('unknown2')) + t.notOk(hasUnknown1, 'unknown1 warnings removed') + t.notOk(hasUnknown2, 'unknown2 warnings removed') +}) + +t.test('loadCommand method', async t => { + const path = t.testdir() + const logs = [] + const logHandler = (...args) => logs.push(args) + process.on('log', logHandler) + t.teardown(() => process.off('log', logHandler)) + + const commandDefs = createDef('cmd-option', { + default: false, + type: Boolean, + description: 'A command-specific option', + }) + + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--cmd-option', '--unknown-cmd'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + config.warn = false + await config.load() + + // Load command-specific definitions + config.loadCommand(commandDefs) + + // Check that cmd-option is now recognized and set to true + t.equal(config.get('cmd-option'), true, 'command option loaded from CLI') + + // Check that warnings were removed for the now-defined key + config.logWarnings() + const warnings = logs.filter(l => l[0] === 'warn' && l[1].includes('cmd-option')) + t.equal(warnings.length, 0, 'no warnings for now-defined cmd-option') + + // Check that unknown-cmd still generates a warning + const unknownWarnings = logs.filter(l => l[0] === 'warn' && l[1].includes('unknown-cmd')) + t.ok(unknownWarnings.length > 0, 'unknown-cmd still generates warning') +}) + +t.test('loadCommand with deprecated definitions', async t => { + const path = t.testdir() + const logs = [] + const logHandler = (...args) => logs.push(args) + process.on('log', logHandler) + t.teardown(() => process.off('log', logHandler)) + + const commandDefs = createDef('deprecated-opt', { + default: 'default', + type: String, + description: 'A deprecated option', + deprecated: 'This option is deprecated', + }) + + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--deprecated-opt', 'value'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + await config.load() + config.loadCommand(commandDefs) + + // Should have deprecation warning + const deprecatedWarnings = logs.filter(l => + l[0] === 'warn' && l[1] === 'config' && l[2] === 'deprecated-opt' + ) + t.ok(deprecatedWarnings.length > 0, 'deprecated option warning logged') +}) + +t.test('getTypesFromDefinitions with no definitions', async t => { + const config = new Config({ + npmPath: t.testdir(), + env: {}, + argv: [process.execPath, __filename], + cwd: process.cwd(), + shorthands, + definitions, + nerfDarts, + }) + + const result = config.getTypesFromDefinitions(undefined) + t.ok(result.types, 'returns types object') + t.ok(result.defaults, 'returns defaults object') + t.ok(result.deprecated, 'returns deprecated object') + t.same(Object.keys(result.types), [], 'empty types for undefined definitions') +}) + +t.test('prefix getter when global is true', async t => { + const path = t.testdir() + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--global'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + await config.load() + t.equal(config.prefix, config.globalPrefix, 'prefix returns globalPrefix when global=true') +}) + +t.test('prefix getter when global is false', async t => { + const path = t.testdir() + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + await config.load() + t.equal(config.prefix, config.localPrefix, 'prefix returns localPrefix when global=false') +}) + +t.test('find throws when config not loaded', async t => { + const config = new Config({ + npmPath: t.testdir(), + env: {}, + argv: [process.execPath, __filename], + cwd: process.cwd(), + shorthands, + definitions, + nerfDarts, + }) + + t.throws( + () => config.find('registry'), + /call config\.load\(\) before reading values/, + 'find throws before load' + ) +}) + +t.test('valid getter with invalid config', async t => { + const path = t.testdir() + const config = new Config({ + npmPath: `${path}/npm`, + env: {}, + argv: [process.execPath, __filename, '--maxsockets', 'not-a-number'], + cwd: path, + shorthands, + definitions, + nerfDarts, + }) + + await config.load() + const isValid = config.valid + t.notOk(isValid, 'config is invalid when it has invalid values') +}) From 333a39dce65d1da8c84ea099cb81dcd64b431156 Mon Sep 17 00:00:00 2001 From: reggi Date: Fri, 28 Nov 2025 10:08:39 -0500 Subject: [PATCH 12/12] try fix failing windows test --- workspaces/config/test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index 17e4f35b34222..c927ae52ba219 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -1144,7 +1144,7 @@ t.test('nerfdart auths set at the top level into the registry', async t => { // now we go ahead and do the repair, and save c.repair() await c.save('user') - t.same(c.list[3], expect) + t.same(c.data.get('user').data, expect) }) } })