doc: improvements to child_process, process docs #5075
Conversation
| @@ -483,7 +499,7 @@ spawn('prg', [], { stdio: ['pipe', null, null, null, 'pipe'] }); | |||
| *It is worth noting that when an IPC channel is established between the | |||
| parent and child processes, and the child is a Node.js process, the child | |||
| is launched with the IPC channel unreferenced (using `unref()`) until the | |||
| child registers an event handler for the `process.on('disconnected')` event. | |||
| child registers an event handler for the [`process.on('disconnect')`][] event. | |||
There was a problem hiding this comment.
Am I correct here? I did not found any mentioning of 'disconnected' event in source, so corrected this.
There was a problem hiding this comment.
Yes, a search of the codebase turns up emit('disconnect'), but not disconnected.
mscdex
added
child_process
|
Seems I have to rebase, okay. |
|
Rebased. |
| without blocking the Node.js event loop. The `child_process.spawnSync()` | ||
| function provides equivalent functionality in a synchronous manner that blocks | ||
| the event loop until the spawned process either exits of is terminated. | ||
| The [`child_process.spawn()`][] method spawns the child process |
There was a problem hiding this comment.
@silverwind fixed
|
This is difficult to review, because of all the line re-wrapping, and the mixture of markup changes, and actual text content changes - I wish those two had been separate PRs. |
| [`child_process.spawn()`][] with the `shell` option set, with | ||
| [`child_process.exec()`][], or by spawning `cmd.exe` and passing the `.bat` or | ||
| `.cmd` file as an argument (which is what the `shell` option and | ||
| [`child_process.exec()`][] do). |
There was a problem hiding this comment.
Its basically impossible to tell during review without an eyeball straining line-by-line review whether you have just added markdown links to the above, or whether you also made some changes to the docs.
The markup changes can get rubber stamped if they look OK after converting to html, text changes need closer review.
There was a problem hiding this comment.
Ok, I'll try to rebase and avoid word wrapping.
|
LGTM |
|
@nodejs/documentation ... can we get another review on this? |
| @@ -3,8 +3,8 @@ | |||
| Stability: 2 - Stable | |||
|
|
|||
| The `child_process` module provides the ability to spawn child processes in | |||
| a manner that is similar, but not identical, to [`popen(3)`][]. This capability | |||
| is primarily provided by the `child_process.spawn()` function: | |||
| a manner that is similar, but not identical, to popen(3). This capability | |||
There was a problem hiding this comment.
Please keep this reference to man pages.
There was a problem hiding this comment.
@eljefedelrodeodeljefe Maybe no need in keeping links anymore thus #5073 landed?
|
With nits concerning LGTM |
|
@estliberitas that's right. Then just the exec(3) change. Thanks! LGTM |
| be launched using `child_process.execFile()`. When running on Windows, `.bat` | ||
| and `.cmd` files can be invoked using `child_process.spawn()` with the `shell` | ||
| option set, with `child_process.exec()`, or by spawning `cmd.exe` and passing | ||
| be launched using [`child_process.execFile()`][]. When running on Windows, `.bat` |
There was a problem hiding this comment.
Unrelated to this PR, but perhaps you can fix: turns out that .bat/.exe/.com are directly executable by spawn, but .cmd, .pl, .js etc. are not. This means that npm installed node scripts are not executable.
There was a problem hiding this comment.
@sam-github Mean, send PR to npm folks?)
|
@sam-github Done. |
|
looks good to me, but you'll have to rebase and resolve the conflicts. |
|
@sam-github done |
|
@estliberitas ... very sorry but this is going to need another rebase. |
|
LGTM |
|
Ok will do in some hrs |
Sort links in lexical order. Add missing links. Add `disconnect` event description in Process doc. Fix typos.
|
Hi. Rebased. |
Sort links in lexical order. Add missing links. Add `disconnect` event description in Process doc. Fix typos. R-URL: #5075 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
|
Landed in f85412d |
Sort links in lexical order. Add missing links. Add `disconnect` event description in Process doc. Fix typos. R-URL: nodejs#5075 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
Sort links in lexical order. Add missing links. Add `disconnect` event description in Process doc. Fix typos. R-URL: #5075 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
Ref: #6911 Ref: #5075 PR-URL: #6952 Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Ref: nodejs#6911 Ref: nodejs#5075 PR-URL: nodejs#6952 Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
|
dropping lts watch as this does not land cleanly... please feel free to open a new PR backporting this directly to |
Ref: #6911 Ref: #5075 PR-URL: #6952 Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Sort links in lexical order. Add missing links.
Add
disconnectevent description in Process doc.Fix typos.