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

workspaces/config/lib/index.js

@@ -210,8 +210,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 +247,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 +544,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 +732,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 +883,7 @@ class ConfigData {
   #raw = null
   constructor (parent) {
     this.#data = Object.create(parent && parent.data)
-    this.#raw = null
+    this.#raw = {}
     this[_valid] = true
   }
 
@@ -897,7 +907,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 +918,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')
+})