worker: make MessagePort constructor non-callable - #28032
Conversation
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
LGTM if CI is green. I guess since worker_threads are still Experimental, this can technically still land as a patch in 12.x even if it is deemed semver-major. (But I'm OK if we'd rather be cautious. Fine either way.)
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/23607/ |
Sorry, something went wrong.
There was a problem hiding this comment.
Can't we just use ConstructorBehavior::kThrow for this? If there is a particular reason for throwing our own style of errors I think that's worth a comment..
Sorry, something went wrong.
There was a problem hiding this comment.
@joyeecheung Yeah, it doesn’t work because that also removes the prototype property. I’m adding a comment, though.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Refactor the C++ code for creating `MessagePort`s to skip calling the constructor and instead directly instantiating the `InstanceTemplate`, and always throw an error from the `MessagePort` constructor. This aligns behaviour with the web, and creating single `MessagePort`s does not make sense anyway.
This comment has been minimized.
Sorry, something went wrong.
Refactor the C++ code for creating `MessagePort`s to skip calling the constructor and instead directly instantiating the `InstanceTemplate`, and always throw an error from the `MessagePort` constructor. This aligns behaviour with the web, and creating single `MessagePort`s does not make sense anyway. PR-URL: nodejs#28032 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refactor the C++ code for creating `MessagePort`s to skip calling the constructor and instead directly instantiating the `InstanceTemplate`, and always throw an error from the `MessagePort` constructor. This aligns behaviour with the web, and creating single `MessagePort`s does not make sense anyway. PR-URL: #28032 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refactor the C++ code for creating MessagePorts to skip calling the
constructor and instead directly instantiating the InstanceTemplate,
and always throw an error from the MessagePort constructor.
This aligns behaviour with the web, and creating single MessagePorts
does not make sense anyway.
This is technically a breaking change and I’d be happy to split out the added throw into a separate PR out of caution, if anybody considers that a good idea.
Checklist