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

Support quoting scripts to run multiple scripts with the same process.env #77

Closed
wants to merge 1 commit into from

Conversation

kentcdodds
Copy link
Owner

This passes all the tests, but I don't think that we should do this. cross-spawn does a LOT more than spawn-command. The only problem is that cross-spawn requires that the args are an array which would require parsing out the command. Unless there's a way to forward the whole string to a command and have that be evaluated... I think eval may work on unix, but what about windows?

Help needed!

@DanReyLop
Copy link
Contributor

DanReyLop commented Mar 5, 2017

crossSpawn('cmd.exe', ['/s', '/c', theWholeCommand]) (works without the .exe too)

That should do the trick, it's basically what spawn-command does. Parsing the command is too big a complication to be worth it IMHO.

@kentcdodds
Copy link
Owner Author

Thanks for chiming in @DanReyLop. Hmmm... Not certain I follow though. Could you make a PR?

@DanReyLop
Copy link
Contributor

DanReyLop commented Mar 6, 2017

I've hacked something together here. Obviously it's not production-ready, but it should illustrate my point. The idea is to wrap everything into a shell, so for example:
cross-env A=asdfg "ls . && ls ."
Output (Unix): sh -c "ls . && ls ."
Output (Win): cmd.exe /c "ls . && ls ."

Using eval (or CALL in Windows) doesn't work because it's a built-in shell command, not an executable command. Invoking a new shell is the closest thing available.

@DanReyLop
Copy link
Contributor

Looking at cross-spawn I've realized there's a way easier solution, using spawn("the whole command", [], { shell: true }), but that was introduced in Node 4.8, so if you want to raise the minimum version required to run cross-env, it's up to you :)

@kentcdodds
Copy link
Owner Author

Sweet! I think that I'm good with raising the minimum requirement to 4.8. Most people should be there (or will be soon) anyway. Besides, the old cross-env works just fine :)

hgwood pushed a commit to hgwood/cross-env that referenced this pull request Mar 15, 2017
Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes kentcdodds#77.

BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted
by the shell.
@hgwood hgwood mentioned this pull request Mar 15, 2017
hgwood pushed a commit to hgwood/cross-env that referenced this pull request Mar 15, 2017
Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes kentcdodds#77.

BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted
by the shell.
kentcdodds pushed a commit that referenced this pull request Mar 15, 2017
* feat(spawn): add support for quoted scripts

Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes #77.


* docs(readme): add gotchas

Add Gotchas paragraph about passing variables to multiple scripts.

* docs(readme): add required node version badge

Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run
cross-env.

* test(spawn): add test for quoted scripts

See #89 (review).


BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.
@hgwood
Copy link
Collaborator

hgwood commented Mar 15, 2017

#89 replaces this PR.

@hgwood hgwood closed this Mar 15, 2017
@kentcdodds kentcdodds deleted the pr/multi-command branch March 15, 2017 16:28
kentcdodds pushed a commit that referenced this pull request Mar 31, 2017
* feat(spawn): add support for quoted scripts

Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes #77.


* docs(readme): add gotchas

Add Gotchas paragraph about passing variables to multiple scripts.

* docs(readme): add required node version badge

Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run
cross-env.

* test(spawn): add test for quoted scripts

See #89 (review).


BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants