-
Notifications
You must be signed in to change notification settings - Fork 75
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
Migrate project from NAN to Node-API for improved compatibility #1088
base: develop
Are you sure you want to change the base?
Conversation
…and fix binding.gyp syntax error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the project from NAN to Node-API to improve compatibility and modernize the native addon bindings. Key changes include:
- Replacing NAN header inclusions with Node-API headers.
- Updating class inheritance and method signatures to align with Node-API constructs.
- Modifying build configurations in binding.gyp to integrate Node-API properly.
Comments suppressed due to low confidence (2)
binding.gyp:31
- The removal of '.' from the include_dirs array may affect the lookup of local headers; verify that this change is intentional and that all required directories are still being included.
<!@(node -p "require(\"node-addon-api\").include")
binding.gyp:132
- [nitpick] The addition of 'scripts/config.js' and the Node-API gyp dependency should be verified to ensure they are necessary for the migration; confirm that these dependencies are available and correctly configured in the project.
'dependencies': [
Just wondering if we could leverage AI agent to convert most of the code from NAPI to Node-API 😄 |
Glad for your reply, |
Can you just check it now, I just modified the following things,
i welcome you with some feedback to it. |
@@ -28,8 +28,7 @@ | |||
'./src/shadow_node.cpp', | |||
], | |||
'include_dirs': [ | |||
'.', | |||
"<!(node -e \"require('nan')\")", | |||
'<!@(node -p "require(\"node-addon-api\").include")', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that a consistent failure on Actions https://github.com/RobotWebTools/rclnodejs/actions/runs/13857494263/job/38777515813?pr=1088#step:7:25, can you pass the compiling locally?
Hello there, This pr is the contribution to issue #1036,
Here's a summary of the changes made to migrate the project from nan to Node-API:
src/addon.cpp
Replaced #include <nan.h> with #include <node_api.h>.
Updated the IsRunningInElectronRenderer function to use Node-API functions instead of nan functions.
Modified the InitModule function to use Node-API for module initialization.
src/shadow_node.hpp
Replaced #include <nan.h> with #include <node_api.h>.
Changed the class inheritance from Nan::ObjectWrap to Napi::ObjectWrap.
Updated method signatures to use Napi::CallbackInfo instead of Nan::FunctionCallbackInfov8::Value.
Replaced Nan::Persistentv8::Function with Napi::FunctionReference.
src/rcl_handle.cpp
Replaced #include <nan.h> with #include <node_api.h>.
Changed the class inheritance from Nan::ObjectWrap to Napi::ObjectWrap.
Updated method signatures to use Napi::CallbackInfo instead of Nan::FunctionCallbackInfov8::Value.
Replaced Nan functions with their Node-API equivalents in the Init, New, SyncProperties, PropertiesGetter, Release, Dismiss, and NewInstance methods.
src/rcl_bindings.cpp
Replaced #include <nan.h> with #include <node_api.h>.
Updated the Init and CreateNode methods to use Node-API functions instead of Nan functions.
src/rcl_action_bindings.cpp
Updated the ActionCreateServer method to use Node-API instead of nan.
If any feedback feel free to mention.