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

2 files changed, 4 insertions, 2 deletions

diff --git a/lib/utils/config/definitions.js b/lib/utils/config/definitions.js
index abe6bda70..36b8a84a6 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 9ee79d192..aaf7fa47e 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
 }