Skip to content

Commit

Permalink
fix(config): consolidate use of npm.color
Browse files Browse the repository at this point in the history
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
  • Loading branch information
wraithgar committed Jul 22, 2021
1 parent 009ad1e commit eb67054
Show file tree
Hide file tree
Showing 10 changed files with 38 additions and 25 deletions.
4 changes: 1 addition & 3 deletions lib/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -87,7 +86,6 @@ class Exec extends BaseCommand {
...flatOptions,
args,
call,
color,
localBin,
locationMsg,
log,
Expand All @@ -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 })
Expand Down
2 changes: 1 addition & 1 deletion lib/fund.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
2 changes: 1 addition & 1 deletion lib/ls.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
8 changes: 7 additions & 1 deletion lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion lib/run-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 []
Expand Down
3 changes: 3 additions & 0 deletions lib/utils/config/definitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
3 changes: 1 addition & 2 deletions lib/utils/setup-log.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -58,6 +59,4 @@ module.exports = (config) => {
log.enableProgress()
else
log.disableProgress()

return enableColorStdout
}
10 changes: 7 additions & 3 deletions test/lib/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
Expand Down Expand Up @@ -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'
})
Expand Down Expand Up @@ -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, [])
Expand Down Expand Up @@ -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 => {
Expand Down
5 changes: 4 additions & 1 deletion test/lib/utils/error-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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()
Expand Down
24 changes: 12 additions & 12 deletions test/lib/utils/setup-log.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]],
Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand Down

0 comments on commit eb67054

Please sign in to comment.