Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

Commit

Permalink
fix: remove env variables that don't exist from the converted command…
Browse files Browse the repository at this point in the history
…s 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
  • Loading branch information
DanReyLop authored and Kent C. Dodds committed Oct 27, 2017
1 parent 36b009e commit 50299d9
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 15 deletions.
2 changes: 1 addition & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const jestConfig = require('kcd-scripts/config').jest

jestConfig.coveragePathIgnorePatterns = jestConfig.coveragePathIgnorePatterns.concat(
['/bin/'],
['/bin/']
)
module.exports = jestConfig
36 changes: 27 additions & 9 deletions src/__tests__/command.js
Original file line number Diff line number Diff line change
@@ -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')
})
12 changes: 10 additions & 2 deletions src/command.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Expand Down

0 comments on commit 50299d9

Please sign in to comment.