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: describe process API for IPC #1978

Merged
merged 1 commit into from
Sep 9, 2015

Conversation

sam-github
Copy link
Contributor

@mscdex mscdex added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Jun 15, 2015

When node is spawned with an IPC channel attached, it can send messages to it's
parent process using `process.send()`. They will be received as a
['message'](child_process.html#child_process_event_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Backticks

Copy link
Member

Choose a reason for hiding this comment

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

nit: They will be received as messages rather than They will be received as a message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye backticks aren't supported there, the markdown syntax I used is consistent with other places in docs where events are linked to.

@Trott thanks for all the comments, I will incorporate

@thefourtheye
Copy link
Contributor

@sam-github Did you get a chance to address the review comments?

@sam-github
Copy link
Contributor Author

comments addressed, @thefourtheye @Trott

@sam-github
Copy link
Contributor Author

@nodejs/collaborators ping, docs for otherwise undocumented node APIs, can I get a thumbs up?


When node is spawned with an IPC channel attached, it can send messages to its
parent process using `process.send()`. Each will be received as a
['message'](child_process.html#child_process_event_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation references should be in this format I think [text][] and then at the bottom of the page, [text]: relative url. PTAL at #2142 for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? There are over a hundred examples in the API docs of the markup above, including one to this specific event.

Separating out to a ref at the bottom is useful when there are multiple uses made of that ref, otherwise I don't see the point.

If there is a stylistic or technical reason to change all markdown refs to the format you used in #2142, writing a tool to convert them all would be a good way, but that is out of scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay. Actually I received a comment in that PR, so I thought that that is the standard which we follow.

Copy link
Member

Choose a reason for hiding this comment

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

@thefourtheye the comment on #2142 was from me. It wasn't about having the link at the bottom of the page, just that the URL needed to be relative (without the https://iojs.org/api/ part). It is fine to have it inline.

Copy link
Contributor

Choose a reason for hiding this comment

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

@targos Ah, I see it now. Thanks for clarifying man :-) @sam-github Sorry about that comment, please ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, thanks for your attention.

@silverwind
Copy link
Contributor

Maybe a small italized disclaimer like *{Method,Event} applies to child processes only.* would be appropriate after the description of each?

@sam-github
Copy link
Contributor Author

@silverwind There is a disclaimer:

If io.js was not spawned with an IPC channel, process.{send, disconnect}() will be undefined.

How would you suggest this be enhanced? Italics around it the entire sentence? "spawned" changed to "spawned as a child process"? You can't get spawned as anything but a child process, so that seems oddly worded to me, but YMMV.

I'm happy to patch in any specific wording or markup to get this wrapped up.

@sam-github
Copy link
Contributor Author

@nodejs/collaborators this is stalled, can I get a review?

When io.js is spawned with an IPC channel attached, it can send messages to its
parent process using `process.send()`. Each will be received as a
['message'](child_process.html#child_process_event_message)
event on the parent's ChildProcess object.
Copy link
Contributor

Choose a reason for hiding this comment

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

ChildProcess should probably be in backticks.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 6, 2015

LGTM minus my comments.

@Fishrock123
Copy link
Contributor

ping @sam-github

@sam-github
Copy link
Contributor Author

I'll have time to try the new merge job this weekend, not enough network ATM.

@Fishrock123
Copy link
Contributor

@sam-github We've suspended using that for now. It wasn't working out to well at the current time.

@@ -903,6 +913,38 @@ a diff reading, useful for benchmarks and measuring intervals:
}, 1000);


## process.send(message[, sendHandle])
Copy link
Member

Choose a reason for hiding this comment

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

as of 607aa3a this should be process.send(message[, sendHandle][, callback]) I believe

@@ -86,7 +86,7 @@ and the `.connected` property is false.
### Event: 'message'

* `message` {Object} a parsed JSON object or primitive value
* `sendHandle` {Handle object} a Socket or Server object
* `sendHandle` {Handle object} a Socket or Server object, or undefined
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: write as net.Socket and net.Server and make them links to the appropriate sections in net.markdown. Ditto in process.markdown.

@bnoordhuis
Copy link
Member

LGTM with suggestions.

@sam-github sam-github force-pushed the doc-process-ipc branch 2 times, most recently from 204d0b5 to 92e55ff Compare September 9, 2015 16:42
@sam-github
Copy link
Contributor Author

@bnoordhuis OK, I also found that when you added the callback it invalidated some refs to child.send() because the anchor link changed. @cjihrig you were working on a link checker, weren't you, how's that going?

@sam-github sam-github merged commit f442904 into nodejs:master Sep 9, 2015
@sam-github sam-github deleted the doc-process-ipc branch September 9, 2015 16:49
@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2015

@cjihrig you were working on a link checker, weren't you, how's that going?

That wasn't me. Maybe @chrisdickinson? A lot of people seem to confuse us.

@AndreasMadsen
Copy link
Member

dgram.Socket (from dgram.createServer()) is also supported.

@Fishrock123
Copy link
Contributor

\o/

sam-github added a commit that referenced this pull request Sep 11, 2015
Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #1978
@rvagg rvagg mentioned this pull request Sep 12, 2015
@Fishrock123 Fishrock123 mentioned this pull request Sep 13, 2015
7 tasks
@tflanagan
Copy link
Contributor

Should references to "io.js" be replaced with "node.js"? I found 2 in a quick scan of these changes.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2015

Yes. Would you care to make a PR @tflanagan

@tflanagan
Copy link
Contributor

#2846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.