fix: save raw data to file, not parsed data

When ${X} values are read from an rc file, those values should be written back as-is when config is re-saved

Fixes #6183

Author: wraithgar

docs/lib/content/commands/npm-config.md

@@ -35,7 +35,7 @@ npm set key=value [key=value...]
 
 Sets each of the config keys to the value provided.
 
-If value is omitted, then it sets it to an empty string.
+If value is omitted, the key will be removed from your config file entirely.
 
 Note: for backwards compatibility, `npm config set key value` is supported
 as an alias for `npm config set key=value`.

lib/commands/config.js

@@ -163,7 +163,13 @@ class Config extends BaseCommand {
           `The \`${baseKey}\` option is deprecated, and can not be set in this way${deprecated}`
         )
       }
-      this.npm.config.set(key, val || '', where)
+
+      if (val === '') {
+        this.npm.config.delete(key, where)
+      } else {
+        this.npm.config.set(key, val, where)
+      }
+
       if (!this.npm.config.validate(where)) {
         log.warn('config', 'omitting invalid config values')
       }

test/lib/commands/config.js

@@ -298,13 +298,13 @@ t.test('config set key1 value1 key2=value2 key3', async t => {
 
   t.equal(sandbox.config.get('access'), 'restricted', 'access was set')
   t.equal(sandbox.config.get('all'), false, 'all was set')
-  t.equal(sandbox.config.get('audit'), false, 'audit was set')
+  t.equal(sandbox.config.get('audit'), true, 'audit was unset and restored to its default')
 
   const contents = await fs.readFile(join(home, '.npmrc'), { encoding: 'utf8' })
   const rc = ini.parse(contents)
   t.equal(rc.access, 'restricted', 'access is set to restricted')
   t.equal(rc.all, false, 'all is set to false')
-  t.equal(rc.audit, false, 'audit is set to false')
+  t.not(contents.includes('audit='), 'config file does not set audit')
 })
 
 t.test('config set invalid key logs warning', async t => {

workspaces/config/lib/index.js

@@ -169,11 +169,7 @@ class Config {
     if (!this.loaded) {
       throw new Error('call config.load() before reading values')
     }
-    // TODO single use?
-    return this.#find(key)
-  }
 
-  #find (key) {
     // have to look in reverse order
     const entries = [...this.data.entries()]
     for (let i = entries.length - 1; i > -1; i--) {
@@ -210,8 +206,11 @@ class Config {
       throw new Error('invalid config location param: ' + where)
     }
     this.#checkDeprecated(key)
-    const { data } = this.data.get(where)
+    const { data, raw } = this.data.get(where)
     data[key] = val
+    if (['global', 'user', 'project'].includes(where)) {
+      raw[key] = val
+    }
 
     // this is now dirty, the next call to this.valid will have to check it
     this.data.get(where)[_valid] = null
@@ -244,7 +243,11 @@ class Config {
     if (!confTypes.has(where)) {
       throw new Error('invalid config location param: ' + where)
     }
-    delete this.data.get(where).data[key]
+    const { data, raw } = this.data.get(where)
+    delete data[key]
+    if (['global', 'user', 'project'].includes(where)) {
+      delete raw[key]
+    }
   }
 
   async load () {
@@ -537,6 +540,7 @@ class Config {
   }
 
   #loadObject (obj, where, source, er = null) {
+    // obj is the raw data read from the file
     const conf = this.data.get(where)
     if (conf.source) {
       const m = `double-loading "${where}" configs from ${source}, ` +
@@ -724,7 +728,9 @@ class Config {
       }
     }
 
-    const iniData = ini.stringify(conf.data).trim() + '\n'
+    // We need the actual raw data before we called parseField so that we are
+    // saving the same content back to the file
+    const iniData = ini.stringify(conf.raw).trim() + '\n'
     if (!iniData.trim()) {
       // ignore the unlink error (eg, if file doesn't exist)
       await unlink(conf.source).catch(er => {})
@@ -873,7 +879,7 @@ class ConfigData {
   #raw = null
   constructor (parent) {
     this.#data = Object.create(parent && parent.data)
-    this.#raw = null
+    this.#raw = {}
     this[_valid] = true
   }
 
@@ -897,7 +903,7 @@ class ConfigData {
   }
 
   set loadError (e) {
-    if (this[_loadError] || this.#raw) {
+    if (this[_loadError] || (Object.keys(this.#raw).length)) {
       throw new Error('cannot set ConfigData loadError after load')
     }
     this[_loadError] = e
@@ -908,7 +914,7 @@ class ConfigData {
   }
 
   set raw (r) {
-    if (this.#raw || this[_loadError]) {
+    if (Object.keys(this.#raw).length || this[_loadError]) {
       throw new Error('cannot set ConfigData raw after load')
     }
     this.#raw = r

workspaces/config/test/index.js

@@ -1317,3 +1317,26 @@ t.test('workspaces', async (t) => {
     t.match(logs[0], ['info', /^found workspace root at/], 'logged info about workspace root')
   })
 })
+
+t.test('env-replaced config from files is not clobbered when saving', async (t) => {
+  const path = t.testdir()
+  const opts = {
+    shorthands: {},
+    argv: ['node', __filename, `--userconfig=${path}/.npmrc`],
+    env: { TEST: 'test value' },
+    definitions: {
+      registry: { default: 'https://registry.npmjs.org/' },
+    },
+    npmPath: process.cwd(),
+  }
+  const c = new Config(opts)
+  await c.load()
+  c.set('test', '${TEST}', 'user')
+  await c.save('user')
+  const d = new Config(opts)
+  await d.load()
+  d.set('other', '${SOMETHING}', 'user')
+  await d.save('user')
+  const rc = readFileSync(`${path}/.npmrc`, 'utf8')
+  t.match(rc, 'test=${TEST}', '${TEST} is present, not parsed')
+})