-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
doc: improve child_process.markdown copy #4383
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
Conversation
doc/api/child_process.markdown
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe replace "created" with "spawned" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are only established when spawn is called with pipe as an option, or fork with silent: true. Unless I misunderstand the diff, this statement seems much more sweeping than is accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clarify that this is the default behavior
|
Left a bunch of comments, but generally LGTM |
doc/api/child_process.markdown
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... "returning the stdout and stderr when the command completes". <--- that last is the very important distinction between the exec* and the spawn/fork.
doc/api/child_process.markdown
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(which is what child_process.exec() does). <-- worth mentioning? to demystify?
|
@jasnell Looks like you addressed all my comments, I remembered a couple other useful tidbits of info, though. |
|
@sam-github ... ok, pushed another set of edits. |
|
@nodejs/collaborators ... can I ask for some additional review and sign off on this? Thank you! |
General improvements to child_process.markdown
3631046 to
f53b65d
Compare
|
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to list the Sync versions right below their asynchronous counterpart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep them grouped by async vs sync
General improvements to child_process.markdown PR-URL: #4383 Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in 7d5c1b5. Can make additional improvements if necessary in a separate PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, this code example doesn't work even on Unix-like platforms. Since there's no shell wrapper,
filemust be an absolute path to executable.argsmust contain all the command line arguments forfile.- All paths within
argsmust be absolute as well. - Stdio redirection is not possible.
Here's an alternative I whipped up that should work on all Unix-like platforms. As always, feel free to use none, some, or all of it.
const execFile = require('child_process').execFile;
const child = execFile('/bin/cat', ['/etc/paths'], (error, stdout, stderr) => {
if (error) {
throw error;
}
console.log(stdout);
});|
@ryansobol ... can I ask you for a quick PR to update the example? :-) Thank you! |
General improvements to child_process.markdown PR-URL: nodejs#4383 Reviewed-By: Colin Ihrig <[email protected]>
|
@jasnell this is not merging cleanly, can you backport this please. |
|
@thealphanerd ..yes, I'll work on backporting all of these kind of doc changes. They may not all make it into v4.2.5 but I'll work on them |
|
@thealphanerd ... I'll be working on porting this to LTS early next week. |
General improvements to child_process.markdown PR-URL: nodejs#4383 Reviewed-By: Colin Ihrig <[email protected]>
General improvements to child_process.markdown PR-URL: #4383 Reviewed-By: Colin Ihrig <[email protected]>
General improvements to child_process.markdown PR-URL: #4383 Reviewed-By: Colin Ihrig <[email protected]>
General improvements to child_process.markdown PR-URL: #4383 Reviewed-By: Colin Ihrig <[email protected]>
General improvements to child_process.markdown PR-URL: nodejs#4383 Reviewed-By: Colin Ihrig <[email protected]>
General improvements to child_process.markdown