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: fix worker example to receive message #21486

Closed

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Jun 23, 2018

require('worker_threads') is not an instance of EventEmitter. So
once method would not be in it. The correct way to receive the message
would be to attach a listener to the message event on the
parentPort.

Also, there is no built-in event called workerMessage. This patch
fixes it by referencing message event.

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

@thefourtheye thefourtheye added doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support. labels Jun 23, 2018
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 23, 2018
@@ -375,7 +376,7 @@ added: v10.5.0
* `transferList` {Object[]}

Send a message to the worker that will be received via
[`require('worker_threads').on('workerMessage')`][].
[`require('worker_threads').parentPort.on('message')`][].
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case the norm is to link worker_threads.parentPort and 'message' separately, to something like

received via [`worker_threads.parentPort`][]'s [`'message'`][`port.on('message')`] event.

@vssenko
Copy link

vssenko commented Jun 29, 2018

Wow, luckly i found this issue, helped a lot. But documentation is still invalid.

@addaleax
Copy link
Member

addaleax commented Jul 9, 2018

@thefourtheye Can you rebase this?

@jasnell
Copy link
Member

jasnell commented Jul 12, 2018

ping @thefourtheye

`require('worker_threads')` is not an instance of `EventEmitter`. So
`on` method would not be in it. The correct way to receive the message
would be to attach a listener to the `message` event on the
`parentPort`.
@thefourtheye
Copy link
Contributor Author

@addaleax @jasnell Sorry for the delay. Rebased the PR now.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
@addaleax
Copy link
Member

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jul 18, 2018
`require('worker_threads')` is not an instance of `EventEmitter`. So
`on` method would not be in it. The correct way to receive the message
would be to attach a listener to the `message` event on the
`parentPort`.

PR-URL: nodejs#21486
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 42be4c3 🎉

@BridgeAR BridgeAR closed this Jul 18, 2018
@thefourtheye thefourtheye deleted the fix-worker-threads-doc branch July 18, 2018 17:14
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 19, 2018
targos pushed a commit that referenced this pull request Jul 19, 2018
`require('worker_threads')` is not an instance of `EventEmitter`. So
`on` method would not be in it. The correct way to receive the message
would be to attach a listener to the `message` event on the
`parentPort`.

PR-URL: #21486
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Jul 31, 2018
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. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants