Skip to content
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

doc: improvements to child_process, process docs #5075

Closed
wants to merge 1 commit into from
Closed

doc: improvements to child_process, process docs #5075

wants to merge 1 commit into from

Conversation

estliberitas
Copy link
Contributor

Sort links in lexical order. Add missing links.
Add disconnect event description in Process doc.
Fix typos.

@@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct here? I did not found any mentioning of 'disconnected' event in source, so corrected this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris @nodejs/ctc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a search of the codebase turns up emit('disconnect'), but not disconnected.

@thefourtheye thefourtheye added the doc Issues and PRs related to the documentations. label Feb 4, 2016
@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. process Issues and PRs related to the process subsystem. labels Feb 4, 2016
@estliberitas
Copy link
Contributor Author

Seems I have to rebase, okay.

@estliberitas
Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silverwind fixed

@sam-github
Copy link
Contributor

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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll try to rebase and avoid word wrapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this reference to man pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eljefedelrodeodeljefe Maybe no need in keeping links anymore thus #5073 landed?

@eljefedelrodeodeljefe
Copy link
Contributor

With nits concerning man page links and a little bit indecisive about: ChildProcess# -> child, but this probably has better readability.

LGTM

@eljefedelrodeodeljefe
Copy link
Contributor

@estliberitas that's right. Then just the exec(3) change. Thanks! LGTM

@estliberitas
Copy link
Contributor Author

@eljefedelrodeodeljefe done

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`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github Mean, send PR to npm folks?)

@estliberitas
Copy link
Contributor Author

@sam-github Done.

@sam-github
Copy link
Contributor

looks good to me, but you'll have to rebase and resolve the conflicts.

@estliberitas
Copy link
Contributor Author

@sam-github done

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

@estliberitas ... very sorry but this is going to need another rebase.
@nodejs/documentation ... can we get a final round of reviews on this one please?

@silverwind
Copy link
Contributor

LGTM

@estliberitas
Copy link
Contributor Author

Ok will do in some hrs

Sort links in lexical order. Add missing links.
Add `disconnect` event description in Process doc.
Fix typos.
@estliberitas
Copy link
Contributor Author

Hi. Rebased.

jasnell pushed a commit that referenced this pull request Apr 22, 2016
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 <jasnell@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@jasnell
Copy link
Member

jasnell commented Apr 22, 2016

Landed in f85412d

@jasnell jasnell closed this Apr 22, 2016
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
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 <jasnell@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
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 <jasnell@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
eljefedelrodeodeljefe pushed a commit that referenced this pull request May 24, 2016
Ref: #6911
Ref: #5075

PR-URL: #6952
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
Ref: nodejs#6911
Ref: nodejs#5075

PR-URL: nodejs#6952
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
@MylesBorins
Copy link
Contributor

dropping lts watch as this does not land cleanly... please feel free to open a new PR backporting this directly to v4.x-staging

rvagg pushed a commit that referenced this pull request Jun 2, 2016
Ref: #6911
Ref: #5075

PR-URL: #6952
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants