Add missing methods for node #1140
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds two new Node APIs—one to retrieve node names with their enclaves and another to get a node’s fully qualified name—along with the necessary bindings and tests.
- Extend C++ bindings to support an “enclaves” flag in
getNodeNamesand exposegetFullyQualifiedName. - Add JS wrapper methods
getNodeNamesAndNamespacesWithEnclavesandgetFullyQualifiedName. - Add corresponding TypeScript and runtime tests for the new methods.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added type assertions for getNodeNamesAndNamespacesEnclaves() and getFullyQualifiedName() |
| test/test-node.js | Added runtime tests for the two new methods |
| src/rcl_node_bindings.cpp | Updated GetNodeNames to accept an enclaves flag and added GetFullyQualifiedName binding |
| lib/node.js | Introduced JS methods getNodeNamesAndNamespacesWithEnclaves and getFullyQualifiedName |
Comments suppressed due to low confidence (1)
test/types/index.test-d.ts:70
- Ensure you have a matching TypeScript declaration for
NodeNamesQueryResultWithEnclaves(including theenclavefield) in your.d.tsfiles so this test compiles.
expectType<rclnodejs.NodeNamesQueryResultWithEnclaves[]>
test/types/index.test-d.ts
Outdated
| expectType<string[]>(node.getNodeNames()); | ||
| expectType<rclnodejs.NodeNamesQueryResult[]>(node.getNodeNamesAndNamespaces()); | ||
| expectType<rclnodejs.NodeNamesQueryResultWithEnclaves[]>( | ||
| node.getNodeNamesAndNamespacesEnclaves() |
There was a problem hiding this comment.
Method name mismatch: the implementation and JS wrapper use getNodeNamesAndNamespacesWithEnclaves(), so this test should call that exact name.
| node.getNodeNamesAndNamespacesEnclaves() | |
| node.getNodeNamesAndNamespacesWithEnclaves() |
|
|
||
| Napi::Value GetNodeNames(const Napi::CallbackInfo& info) { | ||
| Napi::Env env = info.Env(); | ||
|
|
There was a problem hiding this comment.
Validate info.Length() >= 2 and that info[1] is a Boolean before accessing it to prevent out-of-bounds or type errors in the binding.
| if (info.Length() < 2 || !info[1].IsBoolean()) { | |
| Napi::TypeError::New(env, "Expected at least two arguments, with the second being a Boolean.") | |
| .ThrowAsJavaScriptException(); | |
| return env.Null(); | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Adds support for querying node names with enclaves and retrieving a node’s fully qualified name.
- Introduce
getNodeNamesAndNamespacesWithEnclaves()andgetFullyQualifiedName()in the JS API - Implement optional enclave retrieval and new fully qualified name binding in C++
- Add corresponding unit and type-check tests
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added type tests for the new enclave query and fully qualified name methods |
| test/test-node.js | Added runtime tests for getNodeNamesAndNamespacesWithEnclaves and getFullyQualifiedName |
| src/rcl_node_bindings.cpp | Extended GetNodeNames to optionally fetch enclaves; implemented GetFullyQualifiedName |
| lib/node.js | Wrapped the new bindings as getNodeNamesAndNamespacesWithEnclaves and getFullyQualifiedName |
test/types/index.test-d.ts
Outdated
| expectType<string[]>(node.getNodeNames()); | ||
| expectType<rclnodejs.NodeNamesQueryResult[]>(node.getNodeNamesAndNamespaces()); | ||
| expectType<rclnodejs.NodeNamesQueryResultWithEnclaves[]>( | ||
| node.getNodeNamesAndNamespacesEnclaves() |
There was a problem hiding this comment.
The test calls getNodeNamesAndNamespacesEnclaves(), but the actual method is named getNodeNamesAndNamespacesWithEnclaves(). Update the test to use the correct method name.
| node.getNodeNamesAndNamespacesEnclaves() | |
| node.getNodeNamesAndNamespacesWithEnclaves() |
| RCL_RET_OK, | ||
| rcl_get_node_names(node, allocator, &node_names, &node_namespaces), | ||
| "Failed to get_node_names."); | ||
| if (get_enclaves) { |
There was a problem hiding this comment.
[nitpick] The error message used here is generic ("Failed to get_node_names."). Consider updating it to something like "Failed to get node names with enclaves." to clarify which branch failed.
lib/node.js
Outdated
| * Get the fully qualified name of the node. | ||
| * | ||
| * @returns {string} - Return String containing the fully qualified name of the node. |
There was a problem hiding this comment.
[nitpick] The JSDoc comment for getFullyQualifiedName can be simplified for clarity. For example:
/**
* @returns {string} Fully qualified node name.
*/
| * Get the fully qualified name of the node. | |
| * | |
| * @returns {string} - Return String containing the fully qualified name of the node. | |
| * @returns {string} Fully qualified node name. |
Adds support for querying node names with enclaves and retrieving a node’s fully qualified name. - Introduce `getNodeNamesAndNamespacesWithEnclaves()` and `getFullyQualifiedName()` in the JS API - Implement optional enclave retrieval and new fully qualified name binding in C++ - Add corresponding unit and type-check tests Fix: #1139
2 participants
Adds support for querying node names with enclaves and retrieving a node’s fully qualified name.
getNodeNamesAndNamespacesWithEnclaves()andgetFullyQualifiedName()in the JS APIFix: #1139