Skip to content

Default out path to {build}/{configuration} directory and refactored pathSuffix naming strategy #150

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

Merged
merged 5 commits into from
Jul 7, 2025

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Jul 2, 2025

This is to align with the behavior of "cmake-js" to address some of the concerns raised in #146.

Merging this PR will:

  • Change cmake-rn to save the final prebuilds into ./{build}/{configuration} (ex. ./build/Release)
  • Change the Babel plugin to look for the ./build/Release prebuilds when transforming require("bindings")(...) statements.
  • Change the shared code in the host package to strip the sub-path of addons by default to avoid having build-Release as a part of the library name.
  • Refactor the "path-utils" tests, into individually failing tests (which might have to be deleted once Refactor C++ loader: extract cache and addon registry, resolve TODOs #104 merge)

The naming strategy will now be controlled by a pathSuffix option passed to the Babel plugin (the following are the docs for the option):

Controls how the path of the addon inside a package is transformed into a library name.
The transformation is needed to disambiguate and avoid conflicts between addons with the same name (but in different sub-paths or packages).

As an example, if the package name is my-pkg and the path of the addon within the package is build/Release/my-addon.node:

  • "omit": Only the package name is used and the library name will be my-pkg.
  • "strip" (the default): Path gets stripped to its basename and the library name will be my-pkg--my-addon.
  • "keep": The full path is kept and the library name will be my-pkg--build-Release-my-addon.

Or when linking through a --path-suffix option or the NODE_API_PATH_SUFFIX environment variable.

@kraenhansen kraenhansen self-assigned this Jul 2, 2025
Copy link

changeset-bot bot commented Jul 2, 2025

⚠️ No Changeset found

Latest commit: 883978b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kraenhansen
Copy link
Collaborator Author

I was contemplating actually interpreting the --out parameter as a template and replace in the value of the --build and --configuration arguments. This would make it a bit more flexible for users wanting to provide values for --out based on other arguments too 🤔

@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Jul 6, 2025
@kraenhansen kraenhansen changed the title Default out path to {build}/{configuration} directory Default out path to {build}/{configuration} directory and refactored "path suffix" naming strategies Jul 6, 2025
@kraenhansen kraenhansen changed the title Default out path to {build}/{configuration} directory and refactored "path suffix" naming strategies Default out path to {build}/{configuration} directory and refactored pathSuffix naming strategy Jul 6, 2025
@kraenhansen kraenhansen added Host 🏡 Our `react-native-node-api-modules` package Linking 🔗 Discovering and copying prebuilds from packages into the host labels Jul 6, 2025
@kraenhansen kraenhansen requested a review from Copilot July 6, 2025 20:21
Copy link

@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

Aligns the default output directory with cmake-js (./build/{configuration}) and refactors the naming strategy for Node-API modules to use a pathSuffix option instead of a boolean flag.

  • Default --out path is now {build}/{configuration} in the CMake RN CLI.
  • Replaces stripPathSuffix boolean with a pathSuffix enum (omit/strip/keep) across host CLI, Babel plugin, and path-utils.
  • Updates tests, Podspec, and scripts to reflect the new naming strategy and output defaults.

Reviewed Changes

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

Show a summary per file
File Description
path-utils.ts Swapped stripPathSuffix for pathSuffix and updated getLibraryName logic.
path-utils.test.ts Expanded tests for strip, keep, and omit path suffix modes.
cli/program.ts Replaced the old --strip-path-suffix option with --path-suffix.
cli/options.ts Added pathSuffixOption with validation against `"strip"
babel-plugin/plugin.ts Updated plugin to consume pathSuffix and added findNodeAddonForBindings.
babel-plugin/plugin.test.ts Added comprehensive tests for all pathSuffix modes in both require and bindings.
react-native-node-api.podspec Removed legacy --strip-path-suffix logic from the Podspec.
package.json Added --out flag to weak-node-api build scripts.
cmake-rn/src/cli.ts Introduced outPathOption and defaulted globalContext.out to <build>/<configuration>.
Comments suppressed due to low confidence (2)

packages/host/react-native-node-api.podspec:9

  • The Podspec no longer exposes a way to pass --path-suffix when copying frameworks. Consider re-adding logic to read NODE_API_PATH_SUFFIX (or default) and include --path-suffix <strategy> in the CLI invocation.
COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}'"

packages/host/src/node/cli/program.ts:71

  • [nitpick] Previously the CLI warned when stripping suffixes to avoid naming collisions. You may want to reintroduce a warning when pathSuffix is set to strip or omit, as both modes can cause collisions.
  .action(async (pathArg, { force, prune, pathSuffix, android, apple }) => {

* - `"strip"` (default): Path gets stripped to its basename and the library name will be `my-pkg--my-addon`.
* - `"keep"`: The full path is kept and the library name will be `my-pkg--build-Release-my-addon`.
*/
pathSuffix?: "strip" | "omit" | "keep";
};

function assertOptions(opts: unknown): asserts opts is PluginOptions {
Copy link
Preview

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

assertOptions still only checks stripPathSuffix and does not validate the new pathSuffix property. Update it to assert that opts.pathSuffix, if present, is one of 'strip', 'keep', or 'omit', and remove any leftover stripPathSuffix branches.

Copilot uses AI. Check for mistakes.

@@ -82,7 +82,7 @@ const cleanOption = new Option(
const outPathOption = new Option(
"--out <path>",
"Specify the output directory to store the final build artifacts"
);
).default(false, "./{build}/{configuration}");
Copy link
Preview

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

The .default(false, "./{build}/{configuration}") call will set the default to false rather than the intended path string. Use .default("./{build}/{configuration}") so --out defaults correctly.

Suggested change
).default(false, "./{build}/{configuration}");
).default("./{build}/{configuration}");

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is on purpose, since we need the values of build and configuration to construct the default value, we're setting it to false to defer defining its actual value.

@kraenhansen kraenhansen marked this pull request as ready for review July 7, 2025 05:49
@kraenhansen
Copy link
Collaborator Author

I need this merged to progress on the example repo, please review in retrospect 🙏

@kraenhansen kraenhansen merged commit eab4cfc into main Jul 7, 2025
6 checks passed
@kraenhansen kraenhansen deleted the kh/out-to-build branch July 7, 2025 05:50
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.) Host 🏡 Our `react-native-node-api-modules` package Linking 🔗 Discovering and copying prebuilds from packages into the host
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant