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.

Sign up for GitHub

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

Jump to bottom

doc: modernize code examples in the cluster.md #10270

Closed
wants to merge 7 commits into from
Closed

doc: modernize code examples in the cluster.md #10270

wants to merge 7 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Dec 14, 2016 •
edited
Loading

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, cluster

Description of change
  1. var -> let / const
  2. concatenate -> template literal
  3. == -> ===
  4. anonymous function -> arrow function
  5. update an obsolete example (see the comment in the issue)
  6. simplify cluster.workers iteration: Object.keys().forEach -> for...in

Fixes: #10255

@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations. lts-watch-v6.x labels Dec 14, 2016
@vsemozhetbyt vsemozhetbyt mentioned this pull request Dec 14, 2016
Closed
sam-github
sam-github suggested changes Dec 15, 2016
View reviewed changes
doc/api/cluster.md
@@ -30,17 +32,20 @@ if (cluster.isMaster) {
res.writeHead(200);
res.end('hello world\n');
}).listen(8000);

console.log(`Worker ${process.pid} is online`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use "started"? There is an online event, and this isn't 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.

Done.

doc/api/cluster.md
@@ -807,9 +812,9 @@ before last `'disconnect'` or `'exit'` event is emitted.
```js
// Go through all workers
function eachWorker(callback) {
for (var id in cluster.workers) {
Object.keys(cluster.workers).forEach((id) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is worse than original

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@sam-github Should I also change the prototype into the for...in variant?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

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 Done.

@vsemozhetbyt
doc: simplify examples in the cluster.md
e5307f3 
`cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`
@sam-github
Copy link
Contributor

LGTM, should be squashed before landing.

lpinca
lpinca reviewed Dec 15, 2016
View reviewed changes
doc/api/cluster.md
cluster.fork();
}

Object.keys(cluster.workers).forEach((id) => {
for (const id in cluster.workers) {
Copy link
Member

@lpinca lpinca Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: I think it's better to keep Object.keys here? for...in also includes non own enumerable properties.

Copy link
Member

Choose a reason for hiding this comment

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

Nvm for...in is also used below.

Copy link
Contributor

@sam-github sam-github Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

There are no such properties in cluster.workers.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I know, but I think the intention is to also educate in examples?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this example educates that there are no such properties, that cluster.workers is a vanilla js object underived from anything, and doesn't require complex boiler plate safe-guards to be used.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :)

@vsemozhetbyt
Copy link
Contributor Author

ping @nodejs/documentation

@julianduque
Copy link
Contributor

Landed in e03ee71

julianduque pushed a commit that referenced this pull request Dec 22, 2016
@vsemozhetbyt @julianduque
doc: modernize code examples in the cluster.md
e03ee71 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@vsemozhetbyt vsemozhetbyt deleted the cluster.md branch December 22, 2016 18:01
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@vsemozhetbyt @evanlucas
doc: modernize code examples in the cluster.md
19517a5 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@vsemozhetbyt @evanlucas
doc: modernize code examples in the cluster.md
9044423 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@vsemozhetbyt @MylesBorins
doc: modernize code examples in the cluster.md
523e4e1 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@vsemozhetbyt @MylesBorins
doc: modernize code examples in the cluster.md
0fae3d2 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@vsemozhetbyt @MylesBorins
doc: modernize code examples in the cluster.md
c9c9b5c 
- Fixes #10255
- It also will be consistent with a previous code example.
- `cluster.workers` iteration: `Object.keys().forEach` -> `for`...`in`

PR-URL: #10270
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vsemozhetbyt @sam-github @julianduque @lpinca @MylesBorins @nodejs-github-bot