Skip to content

doc: improve worker_threads documentation - #26110

Closed
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:worker-doc
Closed

doc: improve worker_threads documentation#26110
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:worker-doc

Conversation

@addaleax

@addaleaxaddaleax commented Feb 14, 2019

Copy link
Copy Markdown
Member

This adds a few examples and clarifications.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax
doc: improve worker_threads documentation
3e672dd
This adds a few examples and clarifications.
@nodejs-github-bot

nodejs-github-bot commented Feb 14, 2019

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2608/pipeline

@nodejs-github-botnodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 14, 2019
@addaleaxaddaleax added the worker Issues and PRs related to Worker support. label Feb 14, 2019
Trott
Trott reviewed Feb 14, 2019
View reviewed changes
Comment threaddoc/api/worker_threads.md Outdated
Trott
Trott reviewed Feb 14, 2019
View reviewed changes
Comment threaddoc/api/worker_threads.md
This method can be called when no further communication will happen over this
`MessagePort`.

The [`'close'` event][] will be emitted on both `MessagePort` instances that

@TrottTrottFeb 14, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be I'm over-reacting to passive voice, but perhaps this?:

Both `MessagePort` instances that are part of the channel will emit a [`'close'` event][].

@addaleaxaddaleaxFeb 14, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I have to admit that active voice sounds a bit weird to me when referring to events :)

Trott
Trott approved these changes Feb 14, 2019
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left two completely-optional totally-ignorable comment/suggestions, but LGTM as-is.

@Trott

Trott commented Feb 15, 2019

Copy link
Copy Markdown
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2614/

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2019
jasnell
jasnell reviewed Feb 15, 2019
View reviewed changes
Comment threaddoc/api/worker_threads.md
@@ -91,6 +120,16 @@ added: v10.5.0
An arbitrary JavaScript value that contains a clone of the data passed

@jasnelljasnellFeb 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It might be good to include more of a description of what "clone" here means in general. I know there are a few bits and pieces of an explanation through the doc, but coalescing those into a single section with some information on what types of values cannot be cloned, would be good.

@addaleaxaddaleaxFeb 15, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jasnell How does 919fd07 sound to you?

Fishrock123
Fishrock123 reviewed Feb 15, 2019
View reviewed changes
Comment threaddoc/api/worker_threads.md
The `worker_threads` module enables the use of threads with message channels
between them. To access it:
The `worker_threads` module enables the use of threads that execute JS code
in parallel. To access it:

@Fishrock123Fishrock123Feb 15, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe something more like "that allow parallel execution contexts for JS code"?

@addaleaxaddaleaxFeb 15, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@Fishrock123 I’m not sure … “execution context” sounds like a very generic thing?

vsemozhetbyt
vsemozhetbyt approved these changes Feb 16, 2019
View reviewed changes

@vsemozhetbytvsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With nits)

Comment threaddoc/api/worker_threads.md Outdated
Comment threaddoc/api/worker_threads.md Outdated
Comment threaddoc/api/worker_threads.md Outdated
Comment threaddoc/api/worker_threads.md Outdated
Comment threaddoc/api/worker_threads.md Outdated
@addaleax

addaleax commented Feb 17, 2019

Copy link
Copy Markdown
MemberAuthor

@vsemozhetbyt Done!

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2642/

@addaleax

addaleax commented Feb 17, 2019

Copy link
Copy Markdown
MemberAuthor

Landed in 70a500f

@addaleax
addaleax deleted the worker-doc branch February 17, 2019 18:00
addaleax added a commit that referenced this pull request Feb 17, 2019
@addaleax
doc: improve worker_threads documentation
70a500f
This adds a few examples and clarifications.
PR-URL: #26110
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
addaleax added a commit that referenced this pull request Feb 17, 2019
@addaleax
doc: improve worker_threads documentation
896b000
This adds a few examples and clarifications.
PR-URL: #26110
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@BridgeARBridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@addaleax@rvagg
doc: improve worker_threads documentation
a18b847
This adds a few examples and clarifications.
PR-URL: #26110
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@lpincalpincalpinca approved these changes
+3 more reviewers
@Fishrock123Fishrock123Fishrock123 left review comments
@antsmartianantsmartianantsmartian approved these changes
@vsemozhetbytvsemozhetbytvsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.workerIssues and PRs related to Worker support.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@addaleax@nodejs-github-bot@Trott@jasnell@Fishrock123@antsmartian@lpinca@vsemozhetbyt