fix: remove env variables that don't exist from the converted commands in Windows. (#149)

* Remove trailing comma on arguments list (unsupported in Node v6 LTS)

* Remove non-defined env variables from the command (and command args), since those have a different behaviour in Windows.

* Fixed tests

* Added new test cases

* Fixed lint error

Author: DanReyLop

jest.config.js

@@ -1,6 +1,6 @@
 const jestConfig = require('kcd-scripts/config').jest
 
 jestConfig.coveragePathIgnorePatterns = jestConfig.coveragePathIgnorePatterns.concat(
-  ['/bin/'],
+  ['/bin/']
 )
 module.exports = jestConfig

src/__tests__/command.js

@@ -1,56 +1,74 @@
 import isWindowsMock from 'is-windows'
 import commandConvert from '../command'
 
+const env = {
+  test: 'a',
+  test1: 'b',
+  test2: 'c',
+  test3: 'd',
+  'empty_var': ''
+}
+
 beforeEach(() => {
   isWindowsMock.__mock.reset()
 })
 
 test(`converts unix-style env variable usage for windows`, () => {
   isWindowsMock.__mock.returnValue = true
-  expect(commandConvert('$test')).toBe('%test%')
+  expect(commandConvert('$test', env)).toBe('%test%')
 })
 
 test(`leaves command unchanged when not a variable`, () => {
-  expect(commandConvert('test')).toBe('test')
+  expect(commandConvert('test', env)).toBe('test')
 })
 
 test(`doesn't convert windows-style env variable`, () => {
   isWindowsMock.__mock.returnValue = false
-  expect(commandConvert('%test%')).toBe('%test%')
+  expect(commandConvert('%test%', env)).toBe('%test%')
 })
 
 test(`leaves variable unchanged when using correct operating system`, () => {
   isWindowsMock.__mock.returnValue = false
-  expect(commandConvert('$test')).toBe('$test')
+  expect(commandConvert('$test', env)).toBe('$test')
 })
 
 test(`is stateless`, () => {
   // this test prevents falling into regexp traps like this:
   // http://stackoverflow.com/a/1520853/971592
   isWindowsMock.__mock.returnValue = true
-  expect(commandConvert('$test')).toBe(commandConvert('$test'))
+  expect(commandConvert('$test', env)).toBe(commandConvert('$test', env))
 })
 
 test(`converts embedded unix-style env variables usage for windows`, () => {
   isWindowsMock.__mock.returnValue = true
-  expect(commandConvert('$test1/$test2/$test3')).toBe('%test1%/%test2%/%test3%')
+  expect(commandConvert('$test1/$test2/$test3', env)).toBe('%test1%/%test2%/%test3%')
 })
 
 // eslint-disable-next-line max-len
 test(`leaves embedded variables unchanged when using correct operating system`, () => {
   isWindowsMock.__mock.returnValue = false
-  expect(commandConvert('$test1/$test2/$test3')).toBe('$test1/$test2/$test3')
+  expect(commandConvert('$test1/$test2/$test3', env)).toBe('$test1/$test2/$test3')
 })
 
 test(`converts braced unix-style env variable usage for windows`, () => {
   isWindowsMock.__mock.returnValue = true
   // eslint-disable-next-line no-template-curly-in-string
-  expect(commandConvert('${test}')).toBe('%test%')
+  expect(commandConvert('${test}', env)).toBe('%test%')
+})
+
+test(`removes non-existent variables from the converted command`, () => {
+  isWindowsMock.__mock.returnValue = true
+  expect(commandConvert('$test1/$foo/$test2', env)).toBe('%test1%//%test2%')
+})
+
+test(`removes empty variables from the converted command`, () => {
+  isWindowsMock.__mock.returnValue = true
+  expect(commandConvert('$foo/$test/$empty_var', env)).toBe('/%test%/')
 })
 
 test(`normalizes command on windows`, () => {
   isWindowsMock.__mock.returnValue = true
   // index.js calls `commandConvert` with `normalize` param
   // as `true` for command only
-  expect(commandConvert('./cmd.bat', true)).toBe('cmd.bat')
+  expect(commandConvert('./cmd.bat', env, true)).toBe('cmd.bat')
 })

src/command.js

@@ -6,16 +6,24 @@ export default commandConvert
 /**
  * Converts an environment variable usage to be appropriate for the current OS
  * @param {String} command Command to convert
+ * @param {Object} env Map of the current environment variable names and their values
  * @param {boolean} normalize If the command should be normalized using `path`
  * after converting
  * @returns {String} Converted command
  */
-function commandConvert(command, normalize = false) {
+function commandConvert(command, env, normalize = false) {
   if (!isWindows()) {
     return command
   }
   const envUnixRegex = /\$(\w+)|\${(\w+)}/g // $my_var or ${my_var}
-  const convertedCmd = command.replace(envUnixRegex, '%$1$2%')
+  const convertedCmd = command.replace(envUnixRegex, (match, $1, $2) => {
+    const varName = $1 || $2
+    // In Windows, non-existent variables are not replaced by the shell,
+    // so for example "echo %FOO%" will literally print the string "%FOO%", as
+    // opposed to printing an empty string in UNIX. See kentcdodds/cross-env#145
+    // If the env variable isn't defined at runtime, just strip it from the command entirely
+    return env[varName] ? `%${varName}%` : ''
+  })
   // Normalization is required for commands with relative paths
   // For example, `./cmd.bat`. See kentcdodds/cross-env#127
   // However, it should not be done for command arguments.

src/index.js

@@ -8,16 +8,17 @@ const envSetterRegex = /(\w+)=('(.*)'|"(.*)"|(.*))/
 
 function crossEnv(args, options = {}) {
   const [envSetters, command, commandArgs] = parseCommand(args)
+  const env = getEnvVars(envSetters)
   if (command) {
     const proc = spawn(
       // run `path.normalize` for command(on windows)
-      commandConvert(command, true),
+      commandConvert(command, env, true),
       // by default normalize is `false`, so not run for cmd args
-      commandArgs.map(arg => commandConvert(arg)),
+      commandArgs.map(arg => commandConvert(arg, env)),
       {
         stdio: 'inherit',
         shell: options.shell,
-        env: getEnvVars(envSetters),
+        env,
       },
     )
     process.on('SIGTERM', () => proc.kill('SIGTERM'))