Skip to content

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Sep 21, 2025

From the beginning of this project we wanted to keep things compatible with cmake-js and this is still a priority.

At the same time, I find it strange that our default configuration asks developers to inject CMAKE_JS_* into their targets - I think we can do better than that.

Merging this PR will:

  • Default cmake-rn to injecting a WEAK_NODE_QPI_CONFIG definition which developers can include to add the weak-node-api target into their project, when linking their own addon library.
  • Add a --cmake-js flag to cmake-rn to opt-in to injection of the CMAKE_JS_* defines as before.
  • Add an opt-in --weak-node-api option in gyp-to-cmake to generate CMake configurations compatible with the default cmake-rn settings.
  • Refactor gyp-to-cmake transformer to make it easier to define various target parameters.
  • Refactor cmake-rn to allow passing multiple defines with the same "key", which is generally supported by CMake. (Drive-by and not strictly needed for this change).
  • I choose to opt into the explicit weak-node-api linkage in the node-addon-examples and avoid it in the node-tests to have both code-paths exercised in CI. This decision is pretty arbitrary and we might want to switch over node-tests in the future in favor of a specific test for cmake-js compatibility.

As-is (the cmake-js template)

cmake_minimum_required(VERSION 3.15...3.31)
project(your-addon-name-here)

add_compile_definitions(-DNAPI_VERSION=4)

file(GLOB SOURCE_FILES "your-source files-location-here")

add_library(${PROJECT_NAME} SHARED ${SOURCE_FILES} ${CMAKE_JS_SRC})
set_target_properties(${PROJECT_NAME} PROPERTIES PREFIX "" SUFFIX ".node")
target_include_directories(${PROJECT_NAME} PRIVATE ${CMAKE_JS_INC})
target_link_libraries(${PROJECT_NAME} PRIVATE ${CMAKE_JS_LIB})
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)

if(MSVC AND CMAKE_JS_NODELIB_DEF AND CMAKE_JS_NODELIB_TARGET)
  # Generate node.lib
  execute_process(COMMAND ${CMAKE_AR} /def:${CMAKE_JS_NODELIB_DEF} /out:${CMAKE_JS_NODELIB_TARGET} ${CMAKE_STATIC_LINKER_FLAGS})
endif()

The config I suggest

Note: Instead of relying on $CMAKE_JS_* defines, we ask users to include(${WEAK_NODE_API_CONFIG}) which defines a weak-node-api target that they can link their addon with.

cmake_minimum_required(VERSION 3.15...3.31)
project(your-addon-name-here)

include(${WEAK_NODE_API_CONFIG})

file(GLOB SOURCE_FILES "your-source files-location-here")

add_library(${PROJECT_NAME} SHARED ${SOURCE_FILES})
set_target_properties(${PROJECT_NAME} PROPERTIES PREFIX "" SUFFIX ".node")
target_link_libraries(${PROJECT_NAME} PRIVATE weak-node-api)
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)

@kraenhansen kraenhansen self-assigned this Sep 21, 2025
@kraenhansen kraenhansen added enhancement New feature or request Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) CMake RN Our `cmake` wrapping CLI Host 🏡 Our `react-native-node-api-modules` package gyp-to-cmake labels Sep 21, 2025
@kraenhansen kraenhansen force-pushed the kh/weak-node-api-cmake branch from abb960b to 9b75af3 Compare September 21, 2025 09:51
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@kraenhansen kraenhansen force-pushed the kh/weak-node-api-cmake branch from d9f5485 to 8d96678 Compare September 21, 2025 18:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Introduces an explicit weak-node-api linkage workflow, decoupling default behavior from cmake-js style variable injection and adding opt-in flags across tooling.

  • Adds includable WEAK_NODE_API_CONFIG exposing an imported weak-node-api target for addon linkage.
  • Makes cmake-js compatibility optional via --cmake-js and adds --weak-node-api / --cpp / --define-napi-version options to gyp-to-cmake.
  • Refactors definition handling to allow multiple -D entries with same key and restructures transformer for flexible target parameters.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
packages/node-tests/scripts/build-tests.mts Switch to execSync and enable cmake-js mode during test builds.
packages/node-addon-examples/tests/*/CMakeLists.txt Transition examples to weak-node-api includable config usage.
packages/node-addon-examples/package.json Opt-in to weak-node-api aware gyp-to-cmake generation.
packages/host/weak-node-api/weak-node-api.cmake Adds imported weak-node-api target definition template.
packages/gyp-to-cmake/src/transformer.ts Refactors emission logic to support weak-node-api, optional NAPI version, compile features.
packages/gyp-to-cmake/src/cli.ts Adds new CLI options (--weak-node-api, --define-napi-version, --cpp).
packages/cmake-rn/src/weak-node-api.ts Splits variable sets between weak-node-api and cmake-js compatibility.
packages/cmake-rn/src/cli.ts Supports multiple -D entries, adds --cmake-js, refactors definition aggregation.
packages/cmake-rn/README.md Documents new weak-node-api linkage approach.
docs/WEAK-NODE-API.md Introduces dedicated documentation for weak-node-api.
.changeset/*.md Release notes for new minor versions and breaking default define change.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@kraenhansen kraenhansen requested a review from Copilot September 22, 2025 08:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

libraries.push("weak-node-api");
} else {
libraries.push("${CMAKE_JS_LIB}");
escapedSources.push("${CMAKE_JS_SRC}");
Copy link
Preview

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Mutating the escapedSources array in conditional logic makes the code harder to follow. Consider building the complete sources array upfront or using immutable operations.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

@kraenhansen kraenhansen Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah - I think this is cleaner, in this particular case.

@kraenhansen kraenhansen force-pushed the kh/weak-node-api-cmake branch from 7dc57d6 to 9fe44eb Compare September 22, 2025 12:06
@kraenhansen kraenhansen requested a review from Copilot September 22, 2025 19:06
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"endif()",
);
if (!weakNodeApi) {
// This is required by cmake-js to generate the import library for node.lib on Windows
Copy link
Preview

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The comment should be more precise. Change 'This is required by cmake-js' to 'This code is required by cmake-js'.

Suggested change
// This is required by cmake-js to generate the import library for node.lib on Windows
// This code is required by cmake-js to generate the import library for node.lib on Windows

Copilot uses AI. Check for mistakes.

cursor[bot]

This comment was marked as outdated.

@kraenhansen kraenhansen force-pushed the kh/weak-node-api-cmake branch from a387f73 to a7f3d66 Compare September 22, 2025 19:16
@kraenhansen kraenhansen merged commit ff34c45 into main Sep 23, 2025
12 of 13 checks passed
@kraenhansen kraenhansen deleted the kh/weak-node-api-cmake branch September 23, 2025 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) CMake RN Our `cmake` wrapping CLI enhancement New feature or request gyp-to-cmake Host 🏡 Our `react-native-node-api-modules` package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant