Isaacs/set envs fix

lib/config/set-envs.js

@@ -8,11 +8,14 @@
 // and the value is a string or array.  Otherwise return false.
 const envKey = (key, val) => {
   return !/^[/@_]/.test(key) &&
-    (typeof val === 'string' || Array.isArray(val)) &&
-    `npm_config_${key.replace(/-/g, '_').toLowerCase()}`
+    (typeof envVal(val) === 'string') &&
+      `npm_config_${key.replace(/-/g, '_').toLowerCase()}`
 }
 
-const envVal = val => Array.isArray(val) ? val.join('\n\n') : val
+const envVal = val => Array.isArray(val) ? val.map(v => envVal(v)).join('\n\n')
+  : val === null || val === undefined || val === false ? ''
+  : typeof val === 'object' ? null
+  : String(val)
 
 const sameConfigValue = (def, val) =>
   !Array.isArray(val) || !Array.isArray(def) ? def === val
@@ -23,35 +26,66 @@ const sameArrayValue = (def, val) => {
     return false
   }
   for (let i = 0; i < def.length; i++) {
+    /* istanbul ignore next - there are no array configs where the default
+     * is not an empty array, so this loop is a no-op, but it's the correct
+     * thing to do if we ever DO add a config like that. */
     if (def[i] !== val[i]) {
       return false
     }
   }
   return true
 }
 
-const setEnvs = npm => {
-  // The objects in the config.list array are arranged in
-  // a prototype chain, so we can just for/in over the top
-  // of the stack and grab any that don't match the default
-  const { config: { list: [configs] } } = npm
+const setEnv = (env, rawKey, rawVal) => {
+  const val = envVal(rawVal)
+  const key = envKey(rawKey, val)
+  if (key && val !== null) {
+    env[key] = val
+  }
+}
+
+const setEnvs = (npm, env = process.env) => {
+  // This ensures that all npm config values that are not the defaults are
+  // shared appropriately with child processes, without false positives.
+  //
+  // if the key is the default value,
+  //   if the environ is NOT the default value,
+  //     set the environ
+  //   else skip it, it's fine
+  // if the key is NOT the default value,
+  //   if the env is setting it, then leave it (already set)
+  //   otherwise, set the env
+  const { config: { list: [cliConf, envConf] } } = npm
+  const cliSet = new Set(Object.keys(cliConf))
+  const envSet = new Set(Object.keys(envConf))
   const { defaults } = require('./defaults.js')
-  for (const key in configs) {
-    const val = configs[key]
-    const environ = envKey(key, val)
-    if (!sameConfigValue(defaults[key], val) && environ) {
-      process.env[environ] = envVal(val)
+  // the configs form a prototype chain, so we can for/in over cli to
+  // see all the current values, and hasOwnProperty to see if it's
+  // set therre.
+  for (const key in cliConf) {
+    if (sameConfigValue(defaults[key], cliConf[key])) {
+      if (!sameConfigValue(defaults[key], envConf[key])) {
+        // getting set back to the default in the cli config
+        setEnv(env, key, cliConf[key])
+      }
+    } else {
+      // config is not the default
+      if (!(envSet.has(key) && !cliSet.has(key))) {
+        // was not set in the env, so we have to put it there
+        setEnv(env, key, cliConf[key])
+      }
     }
   }
 
-  process.env.npm_execpath = require.main.filename
-  process.env.npm_node_execpath = process.execPath
-  process.env.npm_command = npm.command
+  // also set some other common ones.
+  env.npm_execpath = require.main.filename
+  env.npm_node_execpath = process.execPath
+  env.npm_command = npm.command
 
   // note: this doesn't afect the *current* node process, of course, since
   // it's already started, but it does affect the options passed to scripts.
-  if (configs['node-options']) {
-    process.env.NODE_OPTIONS = configs['node-options']
+  if (cliConf['node-options']) {
+    env.NODE_OPTIONS = cliConf['node-options']
   }
 }
 

package.json

@@ -44,7 +44,7 @@
   "dependencies": {
     "@npmcli/arborist": "^0.0.16",
     "@npmcli/ci-detect": "^1.2.0",
-    "@npmcli/run-script": "^1.4.0",
+    "@npmcli/run-script": "^1.5.0",
     "abbrev": "~1.1.1",
     "ansicolors": "~0.3.2",
     "ansistyles": "~0.1.3",

test/lib/config/set-envs.js

@@ -0,0 +1,165 @@
+const t = require('tap')
+const setEnvs = require('../../../lib/config/set-envs.js')
+const { defaults } = require('../../../lib/config/defaults.js')
+
+t.test('set envs that are not defaults and not already in env', t => {
+  const envConf = Object.create(defaults)
+  const cliConf = Object.create(envConf)
+  const npm = {
+    config: {
+      list: [cliConf, envConf]
+    }
+  }
+  const extras = {
+    npm_execpath: require.main.filename,
+    npm_node_execpath: process.execPath,
+    npm_command: undefined
+  }
+  const env = {}
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create')
+  envConf.call = 'me, maybe'
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create, already in env')
+  delete envConf.call
+  cliConf.call = 'me, maybe'
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_call: 'me, maybe'
+  }, 'set in env, because changed from default in cli')
+  envConf.call = 'me, maybe'
+  cliConf.call = ''
+  cliConf['node-options'] = 'some options for node'
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_call: '',
+    npm_config_node_options: 'some options for node',
+    NODE_OPTIONS: 'some options for node'
+  }, 'set in env, because changed from default in env, back to default in cli')
+  t.end()
+})
+
+t.test('set envs that are not defaults and not already in env, array style', t => {
+  const envConf = Object.create(defaults)
+  const cliConf = Object.create(envConf)
+  const npm = {
+    config: {
+      list: [cliConf, envConf]
+    }
+  }
+  const extras = {
+    npm_execpath: require.main.filename,
+    npm_node_execpath: process.execPath,
+    npm_command: undefined
+  }
+  const env = {}
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create')
+  envConf.omit = ['dev']
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create, already in env')
+  delete envConf.omit
+  cliConf.omit = ['dev', 'optional']
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_omit: 'dev\n\noptional'
+  }, 'set in env, because changed from default in cli')
+  envConf.omit = ['optional', 'peer']
+  cliConf.omit = []
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_omit: ''
+  }, 'set in env, because changed from default in env, back to default in cli')
+  t.end()
+})
+
+t.test('set envs that are not defaults and not already in env, boolean edition', t => {
+  const envConf = Object.create(defaults)
+  const cliConf = Object.create(envConf)
+  const npm = {
+    config: {
+      list: [cliConf, envConf]
+    }
+  }
+  const extras = {
+    npm_execpath: require.main.filename,
+    npm_node_execpath: process.execPath,
+    npm_command: undefined
+  }
+  const env = {}
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create')
+  envConf.audit = false
+  setEnvs(npm, env)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create, already in env')
+  delete envConf.audit
+  cliConf.audit = false
+  cliConf.ignoreObjects = {
+    some: { object: 12345 }
+  }
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_audit: ''
+  }, 'set in env, because changed from default in cli')
+  envConf.audit = false
+  cliConf.audit = true
+  setEnvs(npm, env)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_audit: 'true'
+  }, 'set in env, because changed from default in env, back to default in cli')
+  t.end()
+})
+
+t.test('default to process.env', t => {
+  const envConf = Object.create(defaults)
+  const cliConf = Object.create(envConf)
+  const npm = {
+    config: {
+      list: [cliConf, envConf]
+    }
+  }
+  const extras = {
+    npm_execpath: require.main.filename,
+    npm_node_execpath: process.execPath,
+    npm_command: undefined
+  }
+  const env = {}
+  const envDescriptor = Object.getOwnPropertyDescriptor(process, 'env')
+  Object.defineProperty(process, 'env', {
+    value: env,
+    configurable: true,
+    enumerable: true,
+    writable: true
+  })
+  t.teardown(() => Object.defineProperty(process, env, envDescriptor))
+
+  setEnvs(npm)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create')
+  envConf.audit = false
+  setEnvs(npm)
+  t.strictSame(env, { ...extras }, 'no new environment vars to create, already in env')
+  delete envConf.audit
+  cliConf.audit = false
+  cliConf.ignoreObjects = {
+    some: { object: 12345 }
+  }
+  setEnvs(npm)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_audit: ''
+  }, 'set in env, because changed from default in cli')
+  envConf.audit = false
+  cliConf.audit = true
+  setEnvs(npm)
+  t.strictSame(env, {
+    ...extras,
+    npm_config_audit: 'true'
+  }, 'set in env, because changed from default in env, back to default in cli')
+  t.end()
+})