Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add execa.command() #261

Merged
merged 23 commits into from May 24, 2019
Merged

Add execa.command() #261

merged 23 commits into from May 24, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 21, 2019

This PR is a follow-up on #182 which allowed specifying the command and its arguments as a single string.

It improves the common case where the program contains a space in its path. For example on Windows C:\Program Files\node\node.exe.

With execa(command, [array], [options]), when:

  1. command contains a space
  2. array is defined
  3. options.shell is not true

Then:

  • former behavior: an exception is thrown.
  • new behavior: command is handled as a command with a space in its path.
// Success
execa('echo hello')
// Fail
execa('echo hello', ['world'])
// Success
execa('echo hello', {shell: true})

// Fail
execa('C:\\Program Files\\node\\node.exe')
// Success
execa('C:\\Program Files\\node\\node.exe', [])
// Success
execa('C:\\Program\\ Files\\node\\node.exe')

I've fixed the tests. No changes need to be done to the README since it already implies that no spaces escaping is needed for programs containing a space in their path.

@ehmicky ehmicky mentioned this pull request May 21, 2019
@GMartigny
Copy link
Contributor

I think this should be documented somewhere that [] is required as second argument when your command has spaces in it.

Weird behavior I see while testing:
If I want to escape myself the command execa("fixtures/command\\ with\\ space") it fail anyway. Maybe it's not an expected behavior, just saying.

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 21, 2019

Good point! Fixed.

@GMartigny
Copy link
Contributor

I confirm that there's a bug with path with more than one spaces:

execa("path/with/one\\ space"); // Success
execa("path/with/more \\than\\ one\\ space"); // Fail

This is due to previousToken not being accurate when ignoring some item of the array.

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 21, 2019

This is a different bug. I've opened a PR at #262 to fix it.

test.js Outdated Show resolved Hide resolved
@ehmicky ehmicky changed the title Fix commands with spaces in the path Add execa.command() and execa.command.sync() May 22, 2019
@ehmicky ehmicky changed the title Add execa.command() and execa.command.sync() Add execa.command() May 22, 2019
@ehmicky ehmicky force-pushed the feature/fix-spaces-in-commands branch from 0049816 to 87f4a73 Compare May 22, 2019 16:04
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 22, 2019

Done. I've separated the command feature into two new methods execa.command() and execa.commandSync().

@cliffordfajardo

This comment has been minimized.

@ehmicky

This comment has been minimized.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
ehmicky and others added 3 commits May 23, 2019 02:00
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 23, 2019

Fixed!

@ehmicky ehmicky requested a review from sindresorhus May 23, 2019 09:33
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 24, 2019

Fixed.

@ehmicky ehmicky requested a review from sindresorhus May 24, 2019 08:58
@sindresorhus sindresorhus merged commit 6853316 into master May 24, 2019
@sindresorhus sindresorhus deleted the feature/fix-spaces-in-commands branch May 24, 2019 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants