child_process: add shell option to spawn() #4598
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mscdex
added
the
child_process
| if (options && options.shell) | ||
| file = options.shell; | ||
| // Make a shallow copy so we don't clobber the user's options object | ||
| options = util._extend({}, options); |
There was a problem hiding this comment.
#4593 is about deprecating util._extend(). Maybe use Object.assign() here?
EDIT: Just a suggestion, feel free to ignore.
There was a problem hiding this comment.
I'd prefer to use Object.assign(). I was waiting to see how #4593 played out, but the reaction seems to be good, so I'll replace this.
cjihrig
force-pushed
the
shell
branch
3 times, most recently
from
ce56f29
to
a7c5918
Compare
|
@bnoordhuis I've addressed your comments. |
jasnell
added
the
semver-minor
|
LGTM |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/1195/ EDIT: Test failed on Windows. Trying again. CI: https://ci.nodejs.org/job/node-test-pull-request/1198/ |
|
CI is all green, but I'd like sign off from @bnoordhuis |
|
@bnoordhuis hope you're feeling better. Ping for a review. |
|
@bnoordhuis sorry, one more ping. |
| options = Object.assign({}, options); | ||
|
|
||
| if (options.shell) { | ||
| const command = [file].concat(args).join(' '); |
There was a problem hiding this comment.
Shouldn't double quotes be escaped on Windows?
There was a problem hiding this comment.
Probably. I'll add a test for it too.
There was a problem hiding this comment.
Actually, maybe not:
C:\Users\Colin>cmd.exe /s /c "echo "foo""
"foo"
C:\Users\Colin>cmd.exe /s /c "echo \"foo\""
\"foo\"
There was a problem hiding this comment.
Huh, I wonder what cmd.exe's algorithm for that is. Counting quotes?
There was a problem hiding this comment.
Ah, looks like the /s enables that behavior - http://stackoverflow.com/questions/9866962/what-is-cmd-s-for
There was a problem hiding this comment.
Learned something new today. Thanks.
|
Sorry for the delay. Left some comments. |
|
@bnoordhuis, I think I've addressed all of your comments. |
|
LGTM if the CI is happy. |
This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: nodejs#1009 PR-URL: nodejs#4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
3 tasks
|
/cc @nodejs/lts |
|
I'm +1 for back-port, it improves compatibility between the LTS versions. |
This was referenced Jan 4, 2017
MylesBorins
pushed a commit
that referenced
this pull request
Jan 13, 2017
This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: #1009 PR-URL: #4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Jan 24, 2017
This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: #1009 PR-URL: #4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
MylesBorins
pushed a commit
that referenced
this pull request
Feb 1, 2017
This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: #1009 PR-URL: #4598 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins
added a commit
that referenced
this pull request
Feb 21, 2017
Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796
MylesBorins
added a commit
that referenced
this pull request
Feb 22, 2017
Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796 PR-URL: #10973
This was referenced Feb 28, 2017
imyller
added a commit
to imyller/meta-nodejs
that referenced
this pull request
Mar 2, 2017
Notable Changes:
* child_process: add shell option to spawn() (cjihrig)
nodejs/node#4598
* crypto:
* add ALPN Support (Shigeki Ohtsu)
nodejs/node#2564
* allow adding extra certs to well-known CAs (Sam Roberts)
nodejs/node#9139
* deps:
* v8: expose statistics about heap spaces (Ben Ripkens)
nodejs/node#4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
nodejs/node#5333
* process:
* add `externalMemory` to `process` (Fedor Indutny)
nodejs/node#9587
* add process.cpuUsage() (Patrick Mueller)
nodejs/node#10796
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller
added a commit
to imyller/meta-nodejs
that referenced
this pull request
Mar 2, 2017
Notable Changes:
* child_process: add shell option to spawn() (cjihrig)
nodejs/node#4598
* crypto:
* add ALPN Support (Shigeki Ohtsu)
nodejs/node#2564
* allow adding extra certs to well-known CAs (Sam Roberts)
nodejs/node#9139
* deps:
* v8: expose statistics about heap spaces (Ben Ripkens)
nodejs/node#4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
nodejs/node#5333
* process:
* add `externalMemory` to `process` (Fedor Indutny)
nodejs/node#9587
* add process.cpuUsage() (Patrick Mueller)
nodejs/node#10796
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
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
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
to kentcdodds/cross-env
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.
kentcdodds
pushed a commit
to kentcdodds/cross-env
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds a
shelloption, tospawn()andspawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, forcompatibility with
exec()'sshelloption.Closes #1009