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

worker: reduce MessagePort prototype to documented API #23037

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

  • worker: hide MessagePort init function behind symbol

    This reduces unintended exposure of internals.

  • worker: reduce MessagePort prototype to documented API

    MessagePort is special because it has to be a C++ API
    that is exposed to userland. Therefore, there is a number
    of internal methods on its native prototype; this commit
    reduces this set of methods to only what is documented in
    the API.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This reduces unintended exposure of internals.
`MessagePort` is special because it has to be a C++ API
that is exposed to userland. Therefore, there is a number
of internal methods on its native prototype; this commit
reduces this set of methods to only what is documented in
the API.
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Sep 23, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Sep 23, 2018
lib/internal/worker.js Outdated Show resolved Hide resolved
const {
Worker: WorkerImpl,
getEnvMessagePort,
threadId
threadId,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this triggers my OCD, can we make this consistent with the above destructuring assignment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, this was leftover from addressing the previous review comment … the comma is gone now :)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you.

[
'ref', 'unref', 'start', 'close', 'postMessage', 'onmessage',
'constructor'
].sort());
Copy link
Member

Choose a reason for hiding this comment

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

Nit: why should the array not be pre-sorted as in: [ 'close', 'constructor', 'onmessage', 'postMessage', 'ref', 'start', 'unref' ]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Laziness? ;) Fixed!

@addaleax
Copy link
Member Author

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

Landed in 2d65e67, 0434d27

@addaleax addaleax closed this Sep 25, 2018
@addaleax addaleax deleted the messageport-reduction branch September 25, 2018 22:34
addaleax added a commit that referenced this pull request Sep 25, 2018
This reduces unintended exposure of internals.

PR-URL: #23037
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 25, 2018
`MessagePort` is special because it has to be a C++ API
that is exposed to userland. Therefore, there is a number
of internal methods on its native prototype; this commit
reduces this set of methods to only what is documented in
the API.

PR-URL: #23037
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 27, 2018
This reduces unintended exposure of internals.

PR-URL: #23037
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 27, 2018
`MessagePort` is special because it has to be a C++ API
that is exposed to userland. Therefore, there is a number
of internal methods on its native prototype; this commit
reduces this set of methods to only what is documented in
the API.

PR-URL: #23037
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants