Integrate rclnodejs/ref-napi into rclnodejs #1285
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the ref-napi package directly into the rclnodejs native module as a third-party dependency, eliminating the need for separate npm package installation.
- Vendors the ref-napi package into
third_party/ref-napi/directory - Adds native C++ bindings integration into the main addon module
- Updates all module references to use the vendored version instead of external package
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/ref-napi/src/ref_napi_bindings.h | Header file defining the InitRefNapi function for integration |
| third_party/ref-napi/src/ref_napi_bindings.cpp | Complete C++ implementation of ref-napi functionality adapted for rclnodejs |
| third_party/ref-napi/lib/ref.js | JavaScript wrapper and API implementation for ref-napi |
| third_party/ref-napi/index.js | Entry point module that re-exports the main ref implementation |
| src/addon.cpp | Integration of ref-napi into main addon exports |
| rosidl_gen/templates/message.dot | Updated require path to use vendored ref-napi |
| rosidl_gen/primitive_types.js | Updated require path to use vendored ref-napi |
| binding.gyp | Added ref-napi source files and include paths to build configuration |
Comments suppressed due to low confidence (3)
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Grammar error in error message. Should be 'writeInt32: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Grammar error in error message. Should be 'writeInt64: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Grammar error in error message. Should be 'writeUInt64: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| char* ptr = AddressForArgs(args); | ||
|
|
||
| if (ptr == nullptr) { | ||
| throw TypeError::New(env, "readInt64: Cannot read from nullptr pointer"); |
There was a problem hiding this comment.
Error message says 'readInt64' but this is in the ReadInt32 function. Should be 'readInt32: Cannot read from nullptr pointer'.
| throw TypeError::New(env, "readInt64: Cannot read from nullptr pointer"); | |
| throw TypeError::New(env, "readInt32: Cannot read from nullptr pointer"); |
| char* ptr = AddressForArgs(args); | ||
|
|
||
| if (ptr == nullptr) { | ||
| throw Error::New(env, "readObject: Cannot write to nullptr pointer"); |
There was a problem hiding this comment.
Error message says 'readObject' but this is in the WriteObject function. Should be 'writeObject: Cannot write to nullptr pointer'.
| throw Error::New(env, "readObject: Cannot write to nullptr pointer"); | |
| throw Error::New(env, "writeObject: Cannot write to nullptr pointer"); |
binding.gyp
Outdated
| ], | ||
| 'include_dirs': [ | ||
| '.', | ||
| './third_party/ref-napi/src', |
There was a problem hiding this comment.
Inconsistent indentation. This line should align with the other include_dirs entries using 8 spaces instead of 2.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error - should be 'no digits were found' instead of 'no digits we found'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error - should be 'no digits were found' instead of 'no digits we found'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error - should be 'no digits were found' instead of 'no digits we found'.
// Copyright (c) 2020 The ref-napi Authors.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error. Should be 'writeInt32: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error. Should be 'writeInt64: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
third_party/ref-napi/src/ref_napi_bindings.cpp:1
- Error message has grammatical error. Should be 'writeUInt64: no digits were found in input String'.
// Copyright (c) 2020 The ref-napi Authors.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This PR integrates the `ref-napi` package directly into the rclnodejs native module as a third-party dependency, eliminating the need for separate npm package installation. - Vendors the ref-napi package into `third_party/ref-napi/` directory - Adds native C++ bindings integration into the main addon module - Updates all module references to use the vendored version instead of external package Fix: #1284
2 participants
This PR integrates the
ref-napipackage directly into the rclnodejs native module as a third-party dependency, eliminating the need for separate npm package installation.third_party/ref-napi/directoryFix: #1284