fix: npm config

- Small refactors such as line breaks and favor usage of flatOptions
- Removes validBefore? console.error msg
- Fix typos

PR-URL: #2001
Credit: @ruyadorno
Close: #2001
Reviewed-by: @isaacs

Author: ruyadorno

lib/config.js

@@ -3,13 +3,13 @@ const { defaults, types } = require('./utils/config.js')
 const usageUtil = require('./utils/usage.js')
 const output = require('./utils/output.js')
 
-const editor = require('editor')
 const mkdirp = require('mkdirp-infer-owner')
 const { dirname } = require('path')
 const { promisify } = require('util')
 const fs = require('fs')
 const readFile = promisify(fs.readFile)
 const writeFile = promisify(fs.writeFile)
+const editor = promisify(require('editor'))
 const { EOL } = require('os')
 const ini = require('ini')
 
@@ -28,18 +28,25 @@ const cmd = (args, cb) => config(args).then(() => cb()).catch(cb)
 
 const completion = (opts, cb) => {
   const argv = opts.conf.argv.remain
-  if (argv[1] !== 'config') argv.unshift('config')
+  if (argv[1] !== 'config') {
+    argv.unshift('config')
+  }
+
   if (argv.length === 2) {
     const cmds = ['get', 'set', 'delete', 'ls', 'rm', 'edit']
-    if (opts.partialWord !== 'l') cmds.push('list')
+    if (opts.partialWord !== 'l') {
+      cmds.push('list')
+    }
     return cb(null, cmds)
   }
 
   const action = argv[2]
   switch (action) {
     case 'set':
       // todo: complete with valid values, if possible.
-      if (argv.length > 3) return cb(null, [])
+      if (argv.length > 3) {
+        return cb(null, [])
+      }
       // fallthrough
       /* eslint no-fallthrough:0 */
     case 'get':
@@ -49,12 +56,14 @@ const completion = (opts, cb) => {
     case 'edit':
     case 'list':
     case 'ls':
-      return cb(null, [])
     default:
       return cb(null, [])
   }
 }
 
+const UsageError = () =>
+  Object.assign(new Error(usage), { code: 'EUSAGE' })
+
 const config = async ([action, key, val]) => {
   npm.log.disableProgress()
   try {
@@ -72,13 +81,13 @@ const config = async ([action, key, val]) => {
         break
       case 'list':
       case 'ls':
-        await (npm.config.get('json') ? listJson() : list())
+        await (npm.flatOptions.json ? listJson() : list())
         break
       case 'edit':
         await edit()
         break
       default:
-        throw usage
+        throw UsageError()
     }
   } finally {
     npm.log.enableProgress()
@@ -87,7 +96,7 @@ const config = async ([action, key, val]) => {
 
 const set = async (key, val) => {
   if (key === undefined) {
-    throw usage
+    throw UsageError()
   }
 
   if (val === undefined) {
@@ -103,9 +112,7 @@ const set = async (key, val) => {
   key = key.trim()
   val = val.trim()
   npm.log.info('config', 'set %j %j', key, val)
-  const where = npm.config.get('global') ? 'global' : 'user'
-  const validBefore = npm.config.data.get(where).valid
-  console.error('validBefore?', validBefore)
+  const where = npm.flatOptions.global ? 'global' : 'user'
   npm.config.set(key, val, where)
   if (!npm.config.validate(where)) {
     npm.log.warn('config', 'omitting invalid config values')
@@ -127,18 +134,18 @@ const get = async key => {
 
 const del = async key => {
   if (!key) {
-    throw usage
+    throw UsageError()
   }
 
-  const where = npm.config.get('global') ? 'global' : 'user'
+  const where = npm.flatOptions.global ? 'global' : 'user'
   npm.config.delete(key, where)
   await npm.config.save(where)
 }
 
 const edit = async () => {
   const { editor: e, global } = npm.flatOptions
   if (!e) {
-    throw new Error('No `editor` config or EDITOR envionment variable set')
+    throw new Error('No `editor` config or EDITOR environment variable set')
   }
 
   const where = global ? 'global' : 'user'
@@ -147,10 +154,14 @@ const edit = async () => {
   // save first, just to make sure it's synced up
   // this also removes all the comments from the last time we edited it.
   await npm.config.save(where)
-  const data = (await readFile(file, 'utf8').catch(() => '')).replace(/\r\n/g, '\n')
+
+  const data = (
+    await readFile(file, 'utf8').catch(() => '')
+  ).replace(/\r\n/g, '\n')
   const defData = Object.entries(defaults).reduce((str, [key, val]) => {
     const obj = { [key]: val }
     const i = ini.stringify(obj)
+      .replace(/\r\n/g, '\n') // normalizes output from ini.stringify
       .replace(/\n$/m, '')
       .replace(/^/g, '; ')
       .replace(/\n/g, '\n; ')
@@ -179,9 +190,7 @@ ${defData}
 `.split('\n').join(EOL)
   await mkdirp(dirname(file))
   await writeFile(file, tmpData, 'utf8')
-  await new Promise((res, rej) => {
-    editor(file, { editor: e }, (er) => er ? rej(er) : res())
-  })
+  await editor(file, { editor: e })
 }
 
 const publicVar = k => !/^(\/\/[^:]+:)?_/.test(k)

test/lib/config.js

@@ -5,10 +5,17 @@ const redactCwd = (path) => {
   const normalizePath = p => p
     .replace(/\\+/g, '/')
     .replace(/\r\n/g, '\n')
-  return normalizePath(path)
+  const replaceCwd = p => p
     .replace(new RegExp(normalizePath(process.cwd()), 'g'), '{CWD}')
-    .replace(process.execPath, '/path/to/node')
-    .replace(process.env.HOME, '~/')
+  const cleanupWinPaths = p => p
+    .replace(normalizePath(process.execPath), '/path/to/node')
+    .replace(normalizePath(process.env.HOME), '~/')
+
+  return cleanupWinPaths(
+    replaceCwd(
+      normalizePath(path)
+    )
+  )
 }
 
 t.cleanSnapshot = (str) => redactCwd(str)
@@ -167,10 +174,11 @@ t.test('config list --json', t => {
 t.test('config delete no args', t => {
   config(['delete'], (err) => {
     t.equal(
-      err,
+      err.message,
       'usage instructions',
       'should throw usage error'
     )
+    t.equal(err.code, 'EUSAGE', 'should throw expected error code')
     t.end()
   })
 })
@@ -224,7 +232,7 @@ t.test('config delete key --global', t => {
 t.test('config set no args', t => {
   config(['set'], (err) => {
     t.equal(
-      err,
+      err.message,
       'usage instructions',
       'should throw usage error'
     )
@@ -366,12 +374,11 @@ t.test('config get no args', t => {
 })
 
 t.test('config get key', t => {
-  t.plan(3)
+  t.plan(2)
 
   const npmConfigGet = npm.config.get
-  npm.config.get = (key, where) => {
+  npm.config.get = (key) => {
     t.equal(key, 'foo', 'should use expected key')
-    t.equal(where, 'user', 'should retrieve key from user config by default')
     return 'bar'
   }
 
@@ -389,32 +396,6 @@ t.test('config get key', t => {
   })
 })
 
-t.test('config get key --global', t => {
-  t.plan(3)
-
-  flatOptions.global = true
-  const npmConfigGet = npm.config.get
-  npm.config.get = (key, where) => {
-    t.equal(key, 'foo', 'should use expected global key')
-    t.equal(where, 'global', 'should retrieve key from global config')
-    return 'bar'
-  }
-
-  npm.config.save = where => {
-    throw new Error('should not save')
-  }
-
-  config(['get', 'foo'], (err) => {
-    t.ifError(err, 'npm config get key --global')
-  })
-
-  t.teardown(() => {
-    flatOptions.global = false
-    npm.config.get = npmConfigGet
-    delete npm.config.save
-  })
-})
-
 t.test('config get private key', t => {
   config(['get', '//private-reg.npmjs.org/:_authThoken'], (err) => {
     t.match(