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: #3563
Credit: @wraithgar
Close: #3563
Reviewed-by: @lukekarrys

Author: wraithgar

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 })

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()
 

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')

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
   }

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 []

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',

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
 }

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 <noargs>, 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 => {

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()

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',