Support quoted scripts #89
Conversation
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.
Add Gotchas paragraph about passing variables to multiple scripts.
hgwood
force-pushed
the
feat/shell-option
branch
from
4f01656 to
75450c1
Compare
There was a problem hiding this comment.
Looks great! Could we add just one test that does:
crossEnv(['GREETING=Hi', 'NAME=Joe', 'echo $GREETING && echo $NAME'])And verify that:
expect(crossSpawnMock.spawn).toHaveBeenCalledWith('echo $GREETING && echo $NAME', {
stdio: 'inherit',
shell: true,
env: Object.assign({}, process.env, {
GREETING: 'Hi',
Name: 'Joe',
}),
})I realize that our tests already pretty much do this, but that way we can be explicit about what we support.
Also, I mention adding a badge to indicate node support, but I just realized that we already have a Prerequisites section in the docs. I'm thinking let's remove that section and use the badge :)
Everything else looks great! Thanks :)
| @@ -7,7 +7,7 @@ | |||
| "cross-env": "dist/bin/cross-env.js" | |||
| }, | |||
| "engines": { | |||
| "node": ">=4.0" | |||
| "node": ">=4.8" | |||
There was a problem hiding this comment.
Let's add a note somewhere in the README indicating Node Version support. I expect some people who have been using cross-env today will need to upgrade node before they can continue using it after this change.
How about we use this badge?
[![node-version][node-version-badge]][node]
<!-- down below, with the rest of the links -->
[node-version-badge]: https://img.shields.io/badge/node-%3E%3D%204.8-orange.svg?style=flat-square
Put it after the npm version badge and before the downloads badge. What do you think?
Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run cross-env.
|
All done. |
|
Turns out cross-spawn can mimic the |
|
Oh, that's fantastic! Let's update the README badge and the |
* 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.
Like #77 but using Node.js 4.8's shell option.
No tests because the behavior change only happens inside of
spawn. There is no perceived difference when using the mock.