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 domain.markdown copy #4451

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 28, 2015

General improvements to the domain.markdown copy including
improved examples.

@jasnell jasnell added doc Issues and PRs related to the documentations. domain Issues and PRs related to the domain subsystem. lts-watch-v4.x labels Dec 28, 2015
@jasnell
Copy link
Member Author

jasnell commented Jan 5, 2016

@nodejs/documentation @misterdjules

will be notified, rather than losing the context of the error in the
`process.on('uncaughtException')` handler, or causing the program to
exit immediately with an error code.
the functionality provided by the `domain` modules may rely on it for the
Copy link
Member

Choose a reason for hiding this comment

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

Singular module?

@Qard
Copy link
Member

Qard commented Jan 6, 2016

Some minor nits, but LGTM when those are addressed.

General improvements to the domain.markdown copy including
improved examples.
@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2016

@Qard ... fixed!

For instance, in the example `cluster` application below, worker processes
are intentionally written to randomly throw errors. When these occur, the
worker process shuts down immediately, leading to an ungraceful exit of the
master process:

Choose a reason for hiding this comment

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

It might help to mention why the master process exits with an error in this case. Unless readers are familiar with the cluster module, it's not crystal clear.

@misterdjules
Copy link

@jasnell Thank you for doing that!

I'm not a fan of having the first/only examples tied to using the cluster module. Not because the two don't go well together, but because it adds to the cognitive load when reading the documentation: one has to be familiar with the cluster module to understand the documentation about domains. I understand though that changing that is not in the scope of this PR. So please feel free to ignore my comments on this topic for now :)

Once the rest of my comments are addressed I'll take another look, but this already looks like a great improvement.

@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2016

@misterdjules .. thanks for the comments! I'll update shortly

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. domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants