Add windowsVerbatimArguments docs for child_process.spawn #16299
Conversation
nodejs-github-bot
added
child_process
doc/api/child_process.md
Outdated
| @@ -405,6 +405,7 @@ changes: | |||
| `'/bin/sh'` on UNIX, and `process.env.ComSpec` on Windows. A different | |||
| shell can be specified as a string. See [Shell Requirements][] and | |||
| [Default Windows Shell][]. **Default:** `false` (no shell). | |||
| * `windowsVerbatimArguments` {Boolean} No quoting or escaping of arguments is done on Windows. Ignored on Unix. | |||
There was a problem hiding this comment.
nit: long line. need to wrap at <= 80 chars
doc/api/child_process.md
Outdated
| @@ -405,6 +405,8 @@ changes: | |||
| `'/bin/sh'` on UNIX, and `process.env.ComSpec` on Windows. A different | |||
| shell can be specified as a string. See [Shell Requirements][] and | |||
| [Default Windows Shell][]. **Default:** `false` (no shell). | |||
| * `windowsVerbatimArguments` {Boolean} No quoting or escaping of arguments is | |||
There was a problem hiding this comment.
This is supported on other child process methods. There are some additional details that should probably be included. It is enabled unconditionally when the shell option is true on Windows. This means it's also turned on for exec() for example.
There was a problem hiding this comment.
So, just to make sure, you want me to both:
- Add the doc to the methods that call
spawnunder the hood (execfamily as well asforkfrom the looks of it) - Mention that it is automatically set when
shellis set totrue
There was a problem hiding this comment.
- Add the doc to the methods that call spawn under the hood (exec family as well as fork from the looks of it)
Yes. I'm not a huge fan of it, but we repeat the options in this file a lot.
- Mention that it is automatically set when shell is set to true
Yes, but only on Windows. exec() and execSync() are a bit special because you can't disable shell, and thus cannot disable WVA.
There was a problem hiding this comment.
Ok, so I updated the docs for fork, spawn, and spawnSync since the exec family always enables shell (fork always disables shell, but windowsVerbatimArguments is still able to be overwritten).
Also added in that the default value for windowsVerbatimArguments is false.
doc/api/child_process.md
Outdated
| @@ -339,6 +339,8 @@ changes: | |||
| When this option is provided, it overrides `silent`. If the array variant | |||
| is used, it must contain exactly one item with value `'ipc'` or an error | |||
| will be thrown. For instance `[0, 1, 2, 'ipc']`. | |||
| * `windowsVerbatimArguments` {Boolean} No quoting or escaping of arguments is | |||
There was a problem hiding this comment.
A nit: primitive types are lowercased, so this should be {boolean} here and in other additions,
…Sync, execFile, and fork
|
Any updates? |
|
AFAICT should be good to go with a green linter CI: https://ci.nodejs.org/job/node-test-linter/13043/ |
|
Filled in a wrong ref, new CI: https://ci.nodejs.org/job/node-test-linter/13045/ |
|
Just realized the linter does not verify the docs...anyway a full CI: https://ci.nodejs.org/job/node-test-pull-request/11048/ (should totally get the doc CI setup) |
|
So, it looks like the build for this was potentially manually aborted and that the checks that aren't "completed" won't because the build is aborted. Are there any changes for me to make or is this just waiting for the doc CI to work? |
|
@andrewstucki The windows job got cancelled probably because of the disk space issue that we had over the weekend. New CI: https://ci.nodejs.org/job/node-test-pull-request/11110/ |
|
@joyeecheung so this looks like it's green on the linter, but failing for a bunch of unrelated reasons on the platform-dependent builds. |
|
Landed in 41611f9, thanks! |
doc: Add windowsVerbatimArguments docs for child_process spawn, spawnSync, execFile, and fork PR-URL: #16299 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
doc: Add windowsVerbatimArguments docs for child_process spawn, spawnSync, execFile, and fork PR-URL: nodejs#16299 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
This does not land cleanly in v6.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported |
doc: Add windowsVerbatimArguments docs for child_process spawn, spawnSync, execFile, and fork PR-URL: #16299 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
This resurrects nodejs/node-v0.x-archive#4259.
I changed a bit of the documentation to more closely mirror the docs found in the header http://docs.libuv.org/en/v1.x/process.html by taking the wording from https://nikhilm.github.io/uvbook/processes.html
Checklist
Affected core subsystem(s)
doc