Migrate nan to node-addon-api for implementing the C++ addon #1090

minggangw · 2025-03-27T02:15:00Z

The day when rclnodejs was initialized, nan was the only library designed to simplify the development of native addons, providing a consistent interface for interacting with the V8 JavaScript engine and Node.js APIs across different Node.js versions.

N-API was introduced in Node.js v8.0.0 as an experimental feature to provide a stable Node API for native modules, it's officially supported by Node and allows developing native addons that are independent of the underlying JavaScript engine, and ensures Application Binary Interface (ABI) stability across different Node.js versions and compiler levels. so it is recommended to migrate to N-API for existing projects.

This patch leverages node-addon-api based on Node-API, which provides a C++ object model and exception handling semantics with low overhead, to rewrite the existing C++ bindings, including:

Add node-addon-api into dependencies.
Replace the Nan headers with node-addon-api headers.
Replace nan APIs with node-addon-api equivalents.
Use Napi::Env for environment handling.
Replace Nan::ObjectWrap with Napi::ObjectWrap.
Replace Nan::Persistent with Napi::FunctionReference.
Update error handling and buffer management.

After this patch, we will remove the explicit usage of classes from v8::.

Fix: #1036

coveralls · 2025-03-27T02:43:04Z

coverage: 85.019% (-0.3%) from 85.325%
when pulling 7db7fa4 on minggangw:fix-1036
into 3d3eb17 on RobotWebTools:develop.

Copilot

Pull Request Overview

This PR updates the logging functionality to improve the extraction of caller information from error stacks and revises the build configuration for native modules.

Updated the stack trace parsing logic in lib/logging.js to support both named and anonymous functions.
Changed the conversion of line number to an integer and updated the include directive in binding.gyp to use node-addon-api.

Reviewed Changes

Copilot reviewed 4 out of 19 changed files in this pull request and generated 1 comment.

File	Description
lib/logging.js	Refactored caller info extraction and adjusted line number parsing.
binding.gyp	Updated the include_dirs command to reference node-addon-api.

Files not reviewed (15)

package.json: Language not supported
src/addon.cpp: Language not supported
src/executor.cpp: Language not supported
src/executor.hpp: Language not supported
src/handle_manager.cpp: Language not supported
src/handle_manager.hpp: Language not supported
src/macros.hpp: Language not supported
src/rcl_action_bindings.hpp: Language not supported
src/rcl_bindings.hpp: Language not supported
src/rcl_handle.cpp: Language not supported
src/rcl_handle.hpp: Language not supported
src/rcl_lifecycle_bindings.cpp: Language not supported
src/rcl_lifecycle_bindings.hpp: Language not supported
src/shadow_node.cpp: Language not supported
src/shadow_node.hpp: Language not supported

Comments suppressed due to low confidence (1)

binding.gyp:33

Ensure that node-addon-api is properly added as a dependency and that the command substitution syntax works across all intended build environments.

<!@(node -p "require('node-addon-api').include")

Copilot · 2025-03-28T07:41:16Z

lib/logging.js

      message,
      caller.functionName,
-      caller.lineNumber,
+      parseInt(caller.lineNumber),


Consider specifying the radix when using parseInt, e.g., parseInt(caller.lineNumber, 10), to avoid unexpected behavior.

Suggested change

parseInt(caller.lineNumber),

parseInt(caller.lineNumber, 10),

Copilot

Pull Request Overview

This PR migrates the addon support from using nan to node-addon-api and updates logging functionality to improve caller information extraction.

Replaces nan include with node-addon-api in binding.gyp.
Updates the logging module to initialize caller details with default values and uses regex-based parsing for enhanced stack frame extraction.
Converts the logged line number from a string to an integer.

Reviewed Changes

Copilot reviewed 4 out of 20 changed files in this pull request and generated 1 comment.

File	Description
lib/logging.js	Enhances caller data extraction by updating stack parsing and type conversion
binding.gyp	Modifies include path to use node-addon-api instead of nan

Files not reviewed (16)

package.json: Language not supported
src/addon.cpp: Language not supported
src/executor.cpp: Language not supported
src/executor.hpp: Language not supported
src/handle_manager.cpp: Language not supported
src/handle_manager.hpp: Language not supported
src/macros.hpp: Language not supported
src/rcl_action_bindings.hpp: Language not supported
src/rcl_bindings.hpp: Language not supported
src/rcl_handle.cpp: Language not supported
src/rcl_handle.hpp: Language not supported
src/rcl_lifecycle_bindings.cpp: Language not supported
src/rcl_lifecycle_bindings.hpp: Language not supported
src/rcl_utilities.hpp: Language not supported
src/shadow_node.cpp: Language not supported
src/shadow_node.hpp: Language not supported

Copilot · 2025-04-02T06:50:37Z

lib/logging.js

      message,
      caller.functionName,
-      caller.lineNumber,
+      parseInt(caller.lineNumber, 10),


If caller.lineNumber remains the default 'unknown', parseInt will return NaN. Consider implementing a fallback for non-numeric values to prevent propagation of NaN.

Suggested change

parseInt(caller.lineNumber, 10),

isNaN(parseInt(caller.lineNumber, 10)) ? -1 : parseInt(caller.lineNumber, 10),

Copilot

Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (2)

src/rcl_utilities.hpp:42

Storing the Napi::Env in a thread-local variable may lead to unexpected behavior if it is accessed outside the initializing thread. Ensure that rclnodejs::StoreEnv is reliably called in all entry points before any usage of GetEnv().

static thread_local Napi::Env env = nullptr;

src/macros.hpp:26

The CHECK_OP_AND_THROW_ERROR_IF_NOT_TRUE macro relies on GetEnv() to provide a valid Napi::Env. Ensure that rclnodejs::StoreEnv is invoked before using this macro to avoid null-pointer issues.

Napi::Error::New(rclnodejs::GetEnv(), message).ThrowAsJavaScriptException();

Copilot

Copilot reviewed 21 out of 22 changed files in this pull request and generated no comments.

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (2)

src/rcl_utilities.h:40

Storing a global reference to Napi::Env may lead to issues if used outside its valid lifecycle. Consider reviewing the design to ensure the environment remains valid when accessed from different threads or later in the execution.

inline Napi::Env& GetEnv() { static thread_local Napi::Env env = nullptr; return env; }

lib/logging.js:57

The usage of 'path.basename' suggests that the 'path' module should be imported, but no import statement is present in the diff. Please ensure the module is imported (e.g., const path = require('path');).

this._info.fileName = path.basename(match[2]);

Copilot

Pull Request Overview

This PR migrates the C++ addon from using the deprecated NAN API to Node-API via node-addon-api, ensuring ABI stability and modern error handling. Key changes include updating include directives and conversion of NAN constructs (e.g. ObjectWrap, Persistent) to their Napi equivalents; revising error handling patterns and improving include guard naming consistency.

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/shadow_node.{h,cpp}	Updated class inheritance, method signatures, and error handling from NAN to Napi
src/rcl_utilities.{h,cpp}	Switched include guards and added API for environment storage
src/rcl_lifecycle_bindings.{h,cpp}	Migrated lifecycle binding functions from NAN to Napi and updated header names
src/rcl_handle.{h,cpp}	Replaced NAN ObjectWrap and Persistent usage with Napi::ObjectWrap and FunctionReference
src/executor.{h,cpp}	Updated constructor parameters and error handling routines using Napi
src/addon.cpp	Shifted from NAN to node-addon-api in module initialization and binding functions
Other files (macros, handle_manager, binding.gyp, etc.)	Converted syntax and build configurations to work with Node-API rather than NAN

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (1)

src/rcl_lifecycle_bindings.cpp:104

The lambda capture includes 'node_options', which is not defined; it appears that 'options' defined earlier was intended instead.

auto js_obj = RclHandle::NewInstance(env, state_machine, node_handle, [node, node_options](void* ptr) {

The day when `rclnodejs` was initialized, [nan](https://github.com/nodejs/nan) was the only library designed to simplify the development of native addons, providing a consistent interface for interacting with the V8 JavaScript engine and Node.js APIs across different Node.js versions. [N-API](https://nodejs.org/api/n-api.html#node-api) was introduced in Node.js v8.0.0 as an experimental feature to provide a stable Node API for native modules, it's officially supported by Node and allows developing native addons that are independent of the underlying JavaScript engine, and ensures Application Binary Interface (ABI) stability across different Node.js versions and compiler levels. so it is recommended to migrate to N-API for existing projects. This patch leverages [node-addon-api](https://github.com/nodejs/node-addon-api) based on [Node-API](https://nodejs.org/api/n-api.html), which provides a C++ object model and exception handling semantics with low overhead, to rewrite the existing C++ bindings, including: 1. Add `node-addon-api` into dependencies. 2. Replace the `Nan` headers with `node-addon-api` headers. 3. Replace `nan` APIs with `node-addon-api` equivalents. 4. Use `Napi::Env` for environment handling. 5. Replace `Nan::ObjectWrap` with `Napi::ObjectWrap`. 6. Replace `Nan::Persistent` with `Napi::FunctionReference`. 7. Update error handling and buffer management. After this patch, we will remove the explicit usage of classes from `v8::`. Fix: #1036

minggangw requested a review from Copilot March 28, 2025 07:40

Copilot AI reviewed Mar 28, 2025

View reviewed changes

minggangw changed the title ~~NOT TO MERGE~~ Migrate nan to node-addon-api for implementing the C++ addon Mar 28, 2025

minggangw requested a review from Copilot April 2, 2025 06:50

Copilot AI reviewed Apr 2, 2025

View reviewed changes

minggangw added 4 commits April 9, 2025 13:30

NOT TO MERGE

d1ae6b7

Fix lint error

185d74f

Cleanup

5524df0

Address comments

a58c846

minggangw force-pushed the fix-1036 branch from a70653c to a58c846 Compare April 9, 2025 05:31

minggangw requested a review from Copilot April 9, 2025 05:36

Copilot AI reviewed Apr 9, 2025

View reviewed changes

Rename .hpp to .h

7db7fa4

minggangw requested a review from Copilot April 15, 2025 02:12

Copilot AI reviewed Apr 15, 2025

View reviewed changes

Fix some errors

a23c2d9

minggangw requested a review from Copilot April 21, 2025 06:28

Copilot AI reviewed Apr 21, 2025

View reviewed changes

minggangw merged commit c21bdcd into RobotWebTools:develop Apr 21, 2025
5 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate nan to node-addon-api for implementing the C++ addon #1090

Migrate nan to node-addon-api for implementing the C++ addon #1090

Uh oh!

minggangw commented Mar 27, 2025 •

edited

Loading

Uh oh!

coveralls commented Mar 27, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	parseInt(caller.lineNumber),
	parseInt(caller.lineNumber, 10),

	parseInt(caller.lineNumber, 10),
	isNaN(parseInt(caller.lineNumber, 10)) ? -1 : parseInt(caller.lineNumber, 10),

Migrate nan to node-addon-api for implementing the C++ addon #1090

Migrate nan to node-addon-api for implementing the C++ addon #1090

Uh oh!

Conversation

minggangw commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minggangw commented Mar 27, 2025 •

edited

Loading

coveralls commented Mar 27, 2025 •

edited

Loading