From eb67054c8303348b25f9717c8f82c8d8d494a242 Mon Sep 17 00:00:00 2001 From: Gar Date: Tue, 20 Jul 2021 11:03:38 -0700 Subject: [PATCH] fix(config): consolidate use of npm.color The config value for `color` should never be directly read by npm, or its submodules. The derived value `npm.color` or `npm.flatOptions.color` is what we want to use. This PR consolidates the use of these values, makes sure there is only one place the value is actually calculated, and stops relying on duplicated code in the setup-log.js file for setting one of them. PR-URL: https://github.com/npm/cli/pull/3563 Credit: @wraithgar Close: #3563 Reviewed-by: @lukekarrys --- lib/exec.js | 4 +--- lib/fund.js | 2 +- lib/ls.js | 2 +- lib/npm.js | 8 +++++++- lib/run-script.js | 2 +- lib/utils/config/definitions.js | 3 +++ lib/utils/setup-log.js | 3 +-- test/lib/exec.js | 10 +++++++--- test/lib/utils/error-message.js | 5 ++++- test/lib/utils/setup-log.js | 24 ++++++++++++------------ 10 files changed, 38 insertions(+), 25 deletions(-) diff --git a/lib/exec.js b/lib/exec.js index 959fab66634bd..8c64c2f240581 100644 --- a/lib/exec.js +++ b/lib/exec.js @@ -68,7 +68,6 @@ class Exec extends BaseCommand { async _exec (_args, { locationMsg, path, runPath }) { const args = [..._args] const call = this.npm.config.get('call') - const color = this.npm.config.get('color') const { flatOptions, localBin, @@ -87,7 +86,6 @@ class Exec extends BaseCommand { ...flatOptions, args, call, - color, localBin, locationMsg, log, @@ -103,7 +101,7 @@ class Exec extends BaseCommand { async _execWorkspaces (args, filters) { await this.setWorkspaces(filters) - const color = this.npm.config.get('color') + const color = this.npm.color for (const path of this.workspacePaths) { const locationMsg = await getLocationMsg({ color, path }) diff --git a/lib/fund.js b/lib/fund.js index 92580a756e8af..1e0fa1ecb9d73 100644 --- a/lib/fund.js +++ b/lib/fund.js @@ -109,7 +109,7 @@ class Fund extends ArboristWorkspaceCmd { } printHuman (fundingInfo) { - const color = !!this.npm.color + const color = this.npm.color const unicode = this.npm.config.get('unicode') const seenUrls = new Map() diff --git a/lib/ls.js b/lib/ls.js index 91e9a9dd3dba8..7e41892c53442 100644 --- a/lib/ls.js +++ b/lib/ls.js @@ -67,7 +67,7 @@ class LS extends ArboristWorkspaceCmd { async ls (args) { const all = this.npm.config.get('all') - const color = !!this.npm.color + const color = this.npm.color const depth = this.npm.config.get('depth') const dev = this.npm.config.get('dev') const development = this.npm.config.get('development') diff --git a/lib/npm.js b/lib/npm.js index db3559a384bd7..966d11210c275 100644 --- a/lib/npm.js +++ b/lib/npm.js @@ -232,7 +232,7 @@ const npm = module.exports = new class extends EventEmitter { process.emit('timeEnd', 'npm:load:setTitle') process.emit('time', 'npm:load:setupLog') - this.color = setupLog(this.config) + setupLog(this.config) process.emit('timeEnd', 'npm:load:setupLog') process.env.COLOR = this.color ? '1' : '0' @@ -261,6 +261,12 @@ const npm = module.exports = new class extends EventEmitter { return flat } + get color () { + // This is a special derived value that takes into consideration not only + // the config, but whether or not we are operating in a tty. + return this.flatOptions.color + } + get lockfileVersion () { return 2 } diff --git a/lib/run-script.js b/lib/run-script.js index b94d2fce07180..1daaeb9900de1 100644 --- a/lib/run-script.js +++ b/lib/run-script.js @@ -137,7 +137,7 @@ class RunScript extends BaseCommand { path = path || this.npm.localPrefix const { scripts, name, _id } = await rpj(`${path}/package.json`) const pkgid = _id || name - const color = !!this.npm.color + const color = this.npm.color if (!scripts) return [] diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js index abe6bda70d8bc..36b8a84a61c47 100644 --- a/lib/utils/config/definitions.js +++ b/lib/utils/config/definitions.js @@ -438,6 +438,9 @@ define('cidr', { flatten, }) +// This should never be directly used, the flattened value is the derived value +// and is sent to other modules, and is also exposed as `npm.color` for use +// inside npm itself. define('color', { default: !process.env.NO_COLOR || process.env.NO_COLOR === '0', usage: '--color|--no-color|--color always', diff --git a/lib/utils/setup-log.js b/lib/utils/setup-log.js index 9ee79d192d9ea..aaf7fa47e266d 100644 --- a/lib/utils/setup-log.js +++ b/lib/utils/setup-log.js @@ -18,6 +18,7 @@ module.exports = (config) => { const stderrTTY = process.stderr.isTTY const dumbTerm = process.env.TERM === 'dumb' const stderrNotDumb = stderrTTY && !dumbTerm + // this logic is duplicated in the config 'color' flattener const enableColorStderr = color === 'always' ? true : color === false ? false : stderrTTY @@ -58,6 +59,4 @@ module.exports = (config) => { log.enableProgress() else log.disableProgress() - - return enableColorStdout } diff --git a/test/lib/exec.js b/test/lib/exec.js index 6d99c7959d88e..03a1bedf97e50 100644 --- a/test/lib/exec.js +++ b/test/lib/exec.js @@ -25,6 +25,7 @@ const LOG_WARN = [] let PROGRESS_IGNORED = false const flatOptions = { npxCache: 'npx-cache-dir', + color: false, cache: 'cache-dir', legacyPeerDeps: false, package: [], @@ -109,12 +110,13 @@ t.afterEach(() => { LOG_WARN.length = 0 PROGRESS_IGNORED = false flatOptions.legacyPeerDeps = false - config.color = false + flatOptions.color = false config['script-shell'] = 'shell-cmd' config.package = [] flatOptions.package = [] config.call = '' config.yes = true + npm.color = false npm.localBin = 'local-bin' npm.globalBin = 'global-bin' }) @@ -268,7 +270,8 @@ t.test('npm exec , run interactive shell', t => { t.test('print message with color when tty and not in CI', t => { CI_NAME = null process.stdin.isTTY = true - config.color = true + npm.color = true + flatOptions.color = true run(t, true, () => { t.strictSame(LOG_WARN, []) @@ -1204,7 +1207,8 @@ t.test('workspaces', t => { }) }) - config.color = true + npm.color = true + flatOptions.color = true npm._mockOutputs.length = 0 await new Promise((res, rej) => { exec.execWorkspaces([], ['a'], er => { diff --git a/test/lib/utils/error-message.js b/test/lib/utils/error-message.js index 3fdfb8cc25089..908d70fc3924d 100644 --- a/test/lib/utils/error-message.js +++ b/test/lib/utils/error-message.js @@ -15,6 +15,9 @@ const { resolve } = require('path') const npm = require('../../../lib/npm.js') const CACHE = '/some/cache/dir' npm.config = { + flat: { + color: false, + }, loaded: false, localPrefix: '/some/prefix/dir', get: key => { @@ -467,7 +470,7 @@ t.test('explain ERESOLVE errors', t => { t.matchSnapshot(errorMessage(er, npm)) t.match(EXPLAIN_CALLED, [[ er, - undefined, + false, path.resolve(npm.cache, 'eresolve-report.txt'), ]]) t.end() diff --git a/test/lib/utils/setup-log.js b/test/lib/utils/setup-log.js index 86befe6e29297..7f907bc7e4148 100644 --- a/test/lib/utils/setup-log.js +++ b/test/lib/utils/setup-log.js @@ -84,12 +84,12 @@ t.test('setup with color=always and unicode', t => { t.strictSame(WARN_CALLED, [['ERESOLVE', 'hello', { some: 'object' }]]) WARN_CALLED.length = 0 - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: 'always', unicode: true, progress: false, - })), true) + })) npmlog.warn('ERESOLVE', 'hello', { some: { other: 'object' } }) t.strictSame(EXPLAIN_CALLED, [[{ some: { other: 'object' } }, true, 2]], @@ -125,12 +125,12 @@ t.test('setup with color=true, no unicode, and non-TTY terminal', t => { process.stderr.isTTY = false process.stdout.isTTY = false - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: false, progress: false, heading: 'asdf', - })), false) + })) t.strictSame(settings, { level: 'warn', @@ -156,12 +156,12 @@ t.test('setup with color=true, no unicode, and dumb TTY terminal', t => { process.stdout.isTTY = true process.env.TERM = 'dumb' - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: true, progress: false, heading: 'asdf', - })), true) + })) t.strictSame(settings, { level: 'warn', @@ -187,12 +187,12 @@ t.test('setup with color=true, no unicode, and non-dumb TTY terminal', t => { process.stdout.isTTY = true process.env.TERM = 'totes not dum' - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: true, progress: true, heading: 'asdf', - })), true) + })) t.strictSame(settings, { level: 'warn', @@ -218,12 +218,12 @@ t.test('setup with non-TTY stdout, TTY stderr', t => { process.stdout.isTTY = false process.env.TERM = 'definitely not a dummy' - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: true, progress: true, heading: 'asdf', - })), false) + })) t.strictSame(settings, { level: 'warn', @@ -248,12 +248,12 @@ t.test('setup with TTY stdout, non-TTY stderr', t => { process.stderr.isTTY = false process.stdout.isTTY = true - t.equal(setupLog(config({ + setupLog(config({ loglevel: 'warn', color: true, progress: true, heading: 'asdf', - })), true) + })) t.strictSame(settings, { level: 'warn',