From b04c4f3ab82118cdd3280a3795738fb0ab332998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Wed, 2 Jul 2025 15:39:57 +0200 Subject: [PATCH 1/5] Default out path to {build}/{configuration} directory --- packages/cmake-rn/src/cli.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/cmake-rn/src/cli.ts b/packages/cmake-rn/src/cli.ts index d7d621f5..86a1486b 100644 --- a/packages/cmake-rn/src/cli.ts +++ b/packages/cmake-rn/src/cli.ts @@ -82,7 +82,7 @@ const cleanOption = new Option( const outPathOption = new Option( "--out ", "Specify the output directory to store the final build artifacts" -); +).default(false, "./{build}/{configuration}"); const ndkVersionOption = new Option( "--ndk-version ", @@ -113,12 +113,12 @@ export const program = new Command("cmake-rn") .description("Build React Native Node API modules with CMake") .addOption(verboseOption) .addOption(sourcePathOption) + .addOption(buildPathOption) + .addOption(outPathOption) .addOption(configurationOption) .addOption(tripletOption) .addOption(androidOption) .addOption(appleOption) - .addOption(buildPathOption) - .addOption(outPathOption) .addOption(cleanOption) .addOption(ndkVersionOption) .addOption(androidSdkVersionOption) @@ -169,6 +169,10 @@ export const program = new Command("cmake-rn") } } + if (!globalContext.out) { + globalContext.out = path.join(buildPath, globalContext.configuration); + } + const tripletContext = [...triplets].map((triplet) => { const tripletBuildPath = getTripletBuildPath(buildPath, triplet); return { From d9cda60964feafec6a2204c7d5139f889fbafe4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Wed, 2 Jul 2025 21:43:41 +0200 Subject: [PATCH 2/5] Adjust weak-node-api output directory --- packages/host/package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/host/package.json b/packages/host/package.json index 81831acb..39596848 100644 --- a/packages/host/package.json +++ b/packages/host/package.json @@ -44,8 +44,8 @@ "copy-node-api-headers": "tsx scripts/copy-node-api-headers.ts", "generate-weak-node-api": "tsx scripts/generate-weak-node-api.ts", "generate-weak-node-api-injector": "tsx scripts/generate-weak-node-api-injector.ts", - "build-weak-node-api": "cmake-rn --no-auto-link --no-weak-node-api-linkage --xcframework-extension --source ./weak-node-api", - "build-weak-node-api:all-triplets": "cmake-rn --android --apple --no-auto-link --no-weak-node-api-linkage --xcframework-extension --source ./weak-node-api", + "build-weak-node-api": "cmake-rn --no-auto-link --no-weak-node-api-linkage --xcframework-extension --source ./weak-node-api --out ./weak-node-api", + "build-weak-node-api:all-triplets": "cmake-rn --android --apple --no-auto-link --no-weak-node-api-linkage --xcframework-extension --source ./weak-node-api --out ./weak-node-api", "test": "tsx --test --test-reporter=@reporters/github --test-reporter-destination=stdout --test-reporter=spec --test-reporter-destination=stdout src/node/**/*.test.ts src/node/*.test.ts", "bootstrap": "npm run copy-node-api-headers && npm run generate-weak-node-api-injector && npm run generate-weak-node-api && npm run build-weak-node-api" }, From 62900f2abfaf958eaeb990b98ea2d65b4874ae8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Sun, 6 Jul 2025 20:07:52 +0200 Subject: [PATCH 3/5] Call findNodeAddonForBindings in plugin --- packages/host/src/node/babel-plugin/plugin.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/host/src/node/babel-plugin/plugin.ts b/packages/host/src/node/babel-plugin/plugin.ts index 49733089..0933da92 100644 --- a/packages/host/src/node/babel-plugin/plugin.ts +++ b/packages/host/src/node/babel-plugin/plugin.ts @@ -4,9 +4,14 @@ import path from "node:path"; import type { PluginObj, NodePath } from "@babel/core"; import * as t from "@babel/types"; -import { getLibraryName, isNodeApiModule, NamingStrategy } from "../path-utils"; +import { + getLibraryName, + isNodeApiModule, + findNodeAddonForBindings, + NamingStrategy, +} from "../path-utils"; -type PluginOptions = { +export type PluginOptions = { stripPathSuffix?: boolean; }; @@ -64,10 +69,9 @@ export function plugin(): PluginObj { const [argument] = p.parent.arguments; if (argument.type === "StringLiteral") { const id = argument.value; - const relativePath = path.join(from, id); - // TODO: Support traversing the filesystem to find the Node-API module - if (isNodeApiModule(relativePath)) { - replaceWithRequireNodeAddon(p.parentPath, relativePath, { + const resolvedPath = findNodeAddonForBindings(id, from); + if (typeof resolvedPath === "string") { + replaceWithRequireNodeAddon(p.parentPath, resolvedPath, { stripPathSuffix, }); } From b8b6b924125bf666c242c92200969766fc3e8b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Sun, 6 Jul 2025 20:08:21 +0200 Subject: [PATCH 4/5] Refactor babel plugin tests --- .../host/src/node/babel-plugin/plugin.test.ts | 246 +++++++++++------- 1 file changed, 148 insertions(+), 98 deletions(-) diff --git a/packages/host/src/node/babel-plugin/plugin.test.ts b/packages/host/src/node/babel-plugin/plugin.test.ts index 73e884aa..86a7a6c8 100644 --- a/packages/host/src/node/babel-plugin/plugin.test.ts +++ b/packages/host/src/node/babel-plugin/plugin.test.ts @@ -1,115 +1,165 @@ import assert from "node:assert/strict"; -import { describe, it } from "node:test"; +import { describe, it, TestContext } from "node:test"; import path from "node:path"; import { transformFileSync } from "@babel/core"; -import { plugin } from "./plugin.js"; +import { plugin, PluginOptions } from "./plugin.js"; import { setupTempDirectory } from "../test-utils.js"; -import { getLibraryName } from "../path-utils.js"; + +type TestTransformationOptions = { + files: Record; + inputFilePath: string; + assertion: (code: string) => void; + options?: PluginOptions; +}; + +function itTransforms( + title: string, + { files, inputFilePath, assertion, options = {} }: TestTransformationOptions +) { + it(`transforms ${title}`, (context: TestContext) => { + const tempDirectoryPath = setupTempDirectory(context, files); + const result = transformFileSync( + path.join(tempDirectoryPath, inputFilePath), + { plugins: [[plugin, options]] } + ); + assert(result, "Expected transformation to produce a result"); + const { code } = result; + assert(code, "Expected transformation to produce code"); + assert(assertion(code), `Unexpected code: ${code}`); + }); +} describe("plugin", () => { - it("transforms require calls, regardless", (context) => { - const tempDirectoryPath = setupTempDirectory(context, { - "package.json": `{ "name": "my-package" }`, - "addon-1.apple.node/addon-1.node": - "// This is supposed to be a binary file", - "addon-2.apple.node/addon-2.node": - "// This is supposed to be a binary file", - "addon-1.js": ` - const addon = require('./addon-1.node'); - console.log(addon); - `, - "addon-2.js": ` - const addon = require('./addon-2.node'); - console.log(addon); - `, - "sub-directory/addon-1.js": ` - const addon = require('../addon-1.node'); - console.log(addon); - `, - "addon-1-bindings.js": ` - const addon = require('bindings')('addon-1'); - console.log(addon); - `, - "require-js-file.js": ` - const addon = require('./addon-1.js'); - console.log(addon); - `, + describe("transforming require(...) calls", () => { + itTransforms("a simple call", { + files: { + "package.json": `{ "name": "my-package" }`, + "my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('./my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + assertion: (code) => + code.includes(`requireNodeAddon("my-package--my-addon")`), }); - const ADDON_1_REQUIRE_ARG = getLibraryName( - path.join(tempDirectoryPath, "addon-1"), - { stripPathSuffix: false } - ); - const ADDON_2_REQUIRE_ARG = getLibraryName( - path.join(tempDirectoryPath, "addon-2"), - { stripPathSuffix: false } - ); + itTransforms("from sub-directory", { + files: { + "package.json": `{ "name": "my-package" }`, + "my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "sub-dir/index.js": ` + const addon = require('../my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "sub-dir/index.js", + assertion: (code) => + code.includes(`requireNodeAddon("my-package--my-addon")`), + }); - { - const result = transformFileSync( - path.join(tempDirectoryPath, "./addon-1.js"), - { plugins: [[plugin, { stripPathSuffix: false }]] } - ); - assert(result); - const { code } = result; - assert( - code && code.includes(`requireNodeAddon("${ADDON_1_REQUIRE_ARG}")`), - `Unexpected code: ${code}` - ); - } + itTransforms("addon in sub-directory", { + files: { + "package.json": `{ "name": "my-package" }`, + "sub-dir/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('./sub-dir/my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + assertion: (code) => + code.includes(`requireNodeAddon("my-package--sub-dir-my-addon")`), + }); - { - const result = transformFileSync( - path.join(tempDirectoryPath, "./addon-2.js"), - { plugins: [[plugin, { naming: "hash" }]] } - ); - assert(result); - const { code } = result; - assert( - code && code.includes(`requireNodeAddon("${ADDON_2_REQUIRE_ARG}")`), - `Unexpected code: ${code}` - ); - } + itTransforms( + "and returns package name when passed stripPathSuffix option", + { + files: { + "package.json": `{ "name": "my-package" }`, + "sub-dir/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('./sub-dir/my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { stripPathSuffix: true }, + assertion: (code) => code.includes(`requireNodeAddon("my-package")`), + } + ); - { - const result = transformFileSync( - path.join(tempDirectoryPath, "./sub-directory/addon-1.js"), - { plugins: [[plugin, { naming: "hash" }]] } - ); - assert(result); - const { code } = result; - assert( - code && code.includes(`requireNodeAddon("${ADDON_1_REQUIRE_ARG}")`), - `Unexpected code: ${code}` - ); - } + itTransforms("and does not touch required JS files", { + files: { + "package.json": `{ "name": "my-package" }`, + // TODO: Add a ./my-addon.node to make this test complete + "my-addon.js": "// Some JS file", + "index.js": ` + const addon = require('./my-addon'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { stripPathSuffix: true }, + assertion: (code) => !code.includes("requireNodeAddon"), + }); + }); - { - const result = transformFileSync( - path.join(tempDirectoryPath, "./addon-1-bindings.js"), - { plugins: [[plugin, { naming: "hash" }]] } - ); - assert(result); - const { code } = result; - assert( - code && code.includes(`requireNodeAddon("${ADDON_1_REQUIRE_ARG}")`), - `Unexpected code: ${code}` - ); - } + describe("transforming require('binding')(...) calls", () => { + itTransforms("a simple call", { + files: { + "package.json": `{ "name": "my-package" }`, + "my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('bindings')('my-addon'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + assertion: (code) => + code.includes(`requireNodeAddon("my-package--my-addon")`), + }); + + describe("in 'build/Release'", () => { + itTransforms("a nested addon", { + files: { + "package.json": `{ "name": "my-package" }`, + "build/Release/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('bindings')('my-addon'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + assertion: (code) => + code.includes( + `requireNodeAddon("my-package--build-Release-my-addon")` + ), + }); - { - const result = transformFileSync( - path.join(tempDirectoryPath, "./require-js-file.js"), - { plugins: [[plugin, { naming: "hash" }]] } - ); - assert(result); - const { code } = result; - assert( - code && !code.includes(`requireNodeAddon`), - `Unexpected code: ${code}` - ); - } + itTransforms("strips path suffix when passing stripPathSuffix option", { + files: { + "package.json": `{ "name": "my-package" }`, + "build/Release/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('bindings')('my-addon'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { stripPathSuffix: true }, + assertion: (code) => code.includes(`requireNodeAddon("my-package")`), + }); + }); }); }); From 883978ba46828fd3052441185e7d63f781641d23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kr=C3=A6n=20Hansen?= Date: Sun, 6 Jul 2025 22:45:58 +0200 Subject: [PATCH 5/5] Refactor naming strategy --- packages/host/react-native-node-api.podspec | 3 +- .../host/src/node/babel-plugin/plugin.test.ts | 127 ++++++++---- packages/host/src/node/babel-plugin/plugin.ts | 26 ++- packages/host/src/node/cli/options.ts | 25 ++- packages/host/src/node/cli/program.ts | 194 ++++++++---------- packages/host/src/node/path-utils.test.ts | 86 +++++--- packages/host/src/node/path-utils.ts | 40 +++- 7 files changed, 297 insertions(+), 204 deletions(-) diff --git a/packages/host/react-native-node-api.podspec b/packages/host/react-native-node-api.podspec index 7592a3c1..00f29e4c 100644 --- a/packages/host/react-native-node-api.podspec +++ b/packages/host/react-native-node-api.podspec @@ -6,8 +6,7 @@ require_relative "./scripts/patch-hermes" NODE_PATH ||= `which node`.strip CLI_COMMAND ||= "'#{NODE_PATH}' '#{File.join(__dir__, "dist/node/cli/run.js")}'" -STRIP_PATH_SUFFIX ||= ENV['NODE_API_MODULES_STRIP_PATH_SUFFIX'] === "true" -COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}' #{STRIP_PATH_SUFFIX ? '--strip-path-suffix' : ''}" +COPY_FRAMEWORKS_COMMAND ||= "#{CLI_COMMAND} link --apple '#{Pod::Config.instance.installation_root}'" # We need to run this now to ensure the xcframeworks are copied vendored_frameworks are considered XCFRAMEWORKS_DIR ||= File.join(__dir__, "xcframeworks") diff --git a/packages/host/src/node/babel-plugin/plugin.test.ts b/packages/host/src/node/babel-plugin/plugin.test.ts index 86a7a6c8..349864b9 100644 --- a/packages/host/src/node/babel-plugin/plugin.test.ts +++ b/packages/host/src/node/babel-plugin/plugin.test.ts @@ -10,7 +10,7 @@ import { setupTempDirectory } from "../test-utils.js"; type TestTransformationOptions = { files: Record; inputFilePath: string; - assertion: (code: string) => void; + assertion(code: string): void; options?: PluginOptions; }; @@ -27,10 +27,26 @@ function itTransforms( assert(result, "Expected transformation to produce a result"); const { code } = result; assert(code, "Expected transformation to produce code"); - assert(assertion(code), `Unexpected code: ${code}`); + assertion(code); }); } +function assertIncludes(needle: string, { reverse = false } = {}) { + return (code: string) => { + if (reverse) { + assert( + !code.includes(needle), + `Expected code to not include: ${needle}, got ${code}` + ); + } else { + assert( + code.includes(needle), + `Expected code to include: ${needle}, got ${code}` + ); + } + }; +} + describe("plugin", () => { describe("transforming require(...) calls", () => { itTransforms("a simple call", { @@ -44,8 +60,7 @@ describe("plugin", () => { `, }, inputFilePath: "index.js", - assertion: (code) => - code.includes(`requireNodeAddon("my-package--my-addon")`), + assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`), }); itTransforms("from sub-directory", { @@ -59,42 +74,57 @@ describe("plugin", () => { `, }, inputFilePath: "sub-dir/index.js", - assertion: (code) => - code.includes(`requireNodeAddon("my-package--my-addon")`), + assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`), }); - itTransforms("addon in sub-directory", { - files: { - "package.json": `{ "name": "my-package" }`, - "sub-dir/my-addon.apple.node/my-addon.node": - "// This is supposed to be a binary file", - "index.js": ` - const addon = require('./sub-dir/my-addon.node'); - console.log(addon); - `, - }, - inputFilePath: "index.js", - assertion: (code) => - code.includes(`requireNodeAddon("my-package--sub-dir-my-addon")`), - }); + describe("in 'sub-dir'", () => { + itTransforms("a nested addon (keeping suffix)", { + files: { + "package.json": `{ "name": "my-package" }`, + "sub-dir/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('./sub-dir/my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { pathSuffix: "keep" }, + assertion: assertIncludes( + `requireNodeAddon("my-package--sub-dir-my-addon")` + ), + }); - itTransforms( - "and returns package name when passed stripPathSuffix option", - { + itTransforms("a nested addon (stripping suffix)", { files: { "package.json": `{ "name": "my-package" }`, "sub-dir/my-addon.apple.node/my-addon.node": "// This is supposed to be a binary file", "index.js": ` - const addon = require('./sub-dir/my-addon.node'); - console.log(addon); - `, + const addon = require('./sub-dir/my-addon.node'); + console.log(addon); + `, }, inputFilePath: "index.js", - options: { stripPathSuffix: true }, - assertion: (code) => code.includes(`requireNodeAddon("my-package")`), - } - ); + options: { pathSuffix: "strip" }, + assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`), + }); + + itTransforms("a nested addon (omitting suffix)", { + files: { + "package.json": `{ "name": "my-package" }`, + "sub-dir/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('./sub-dir/my-addon.node'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { pathSuffix: "omit" }, + assertion: assertIncludes(`requireNodeAddon("my-package")`), + }); + }); itTransforms("and does not touch required JS files", { files: { @@ -107,8 +137,7 @@ describe("plugin", () => { `, }, inputFilePath: "index.js", - options: { stripPathSuffix: true }, - assertion: (code) => !code.includes("requireNodeAddon"), + assertion: assertIncludes("requireNodeAddon", { reverse: true }), }); }); @@ -124,12 +153,28 @@ describe("plugin", () => { `, }, inputFilePath: "index.js", - assertion: (code) => - code.includes(`requireNodeAddon("my-package--my-addon")`), + assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`), }); describe("in 'build/Release'", () => { - itTransforms("a nested addon", { + itTransforms("a nested addon (keeping suffix)", { + files: { + "package.json": `{ "name": "my-package" }`, + "build/Release/my-addon.apple.node/my-addon.node": + "// This is supposed to be a binary file", + "index.js": ` + const addon = require('bindings')('my-addon'); + console.log(addon); + `, + }, + inputFilePath: "index.js", + options: { pathSuffix: "keep" }, + assertion: assertIncludes( + `requireNodeAddon("my-package--build-Release-my-addon")` + ), + }); + + itTransforms("a nested addon (stripping suffix)", { files: { "package.json": `{ "name": "my-package" }`, "build/Release/my-addon.apple.node/my-addon.node": @@ -140,13 +185,11 @@ describe("plugin", () => { `, }, inputFilePath: "index.js", - assertion: (code) => - code.includes( - `requireNodeAddon("my-package--build-Release-my-addon")` - ), + options: { pathSuffix: "strip" }, + assertion: assertIncludes(`requireNodeAddon("my-package--my-addon")`), }); - itTransforms("strips path suffix when passing stripPathSuffix option", { + itTransforms("a nested addon (omitting suffix)", { files: { "package.json": `{ "name": "my-package" }`, "build/Release/my-addon.apple.node/my-addon.node": @@ -157,8 +200,8 @@ describe("plugin", () => { `, }, inputFilePath: "index.js", - options: { stripPathSuffix: true }, - assertion: (code) => code.includes(`requireNodeAddon("my-package")`), + options: { pathSuffix: "omit" }, + assertion: assertIncludes(`requireNodeAddon("my-package")`), }); }); }); diff --git a/packages/host/src/node/babel-plugin/plugin.ts b/packages/host/src/node/babel-plugin/plugin.ts index 0933da92..92792910 100644 --- a/packages/host/src/node/babel-plugin/plugin.ts +++ b/packages/host/src/node/babel-plugin/plugin.ts @@ -9,19 +9,27 @@ import { isNodeApiModule, findNodeAddonForBindings, NamingStrategy, + PathSuffixChoice, + assertPathSuffix, } from "../path-utils"; export type PluginOptions = { - stripPathSuffix?: boolean; + /** + * 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"` (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?: PathSuffixChoice; }; function assertOptions(opts: unknown): asserts opts is PluginOptions { assert(typeof opts === "object" && opts !== null, "Expected an object"); - if ("stripPathSuffix" in opts) { - assert( - typeof opts.stripPathSuffix === "boolean", - "Expected 'stripPathSuffix' to be a boolean" - ); + if ("pathSuffix" in opts) { + assertPathSuffix(opts.pathSuffix); } } @@ -49,7 +57,7 @@ export function plugin(): PluginObj { visitor: { CallExpression(p) { assertOptions(this.opts); - const { stripPathSuffix = false } = this.opts; + const { pathSuffix = "strip" } = this.opts; if (typeof this.filename !== "string") { // This transformation only works when the filename is known return; @@ -72,7 +80,7 @@ export function plugin(): PluginObj { const resolvedPath = findNodeAddonForBindings(id, from); if (typeof resolvedPath === "string") { replaceWithRequireNodeAddon(p.parentPath, resolvedPath, { - stripPathSuffix, + pathSuffix, }); } } @@ -81,7 +89,7 @@ export function plugin(): PluginObj { isNodeApiModule(path.join(from, id)) ) { const relativePath = path.join(from, id); - replaceWithRequireNodeAddon(p, relativePath, { stripPathSuffix }); + replaceWithRequireNodeAddon(p, relativePath, { pathSuffix }); } } }, diff --git a/packages/host/src/node/cli/options.ts b/packages/host/src/node/cli/options.ts index 84a32acd..3e8b512b 100644 --- a/packages/host/src/node/cli/options.ts +++ b/packages/host/src/node/cli/options.ts @@ -1,16 +1,15 @@ -import assert from "node:assert/strict"; - import { Option } from "@commander-js/extra-typings"; -const { NODE_API_MODULES_STRIP_PATH_SUFFIX } = process.env; -assert( - typeof NODE_API_MODULES_STRIP_PATH_SUFFIX === "undefined" || - NODE_API_MODULES_STRIP_PATH_SUFFIX === "true" || - NODE_API_MODULES_STRIP_PATH_SUFFIX === "false", - "Expected NODE_API_MODULES_STRIP_PATH_SUFFIX to be either 'true' or 'false'" -); +import { assertPathSuffix, PATH_SUFFIX_CHOICES } from "../path-utils"; + +const { NODE_API_PATH_SUFFIX } = process.env; +if (typeof NODE_API_PATH_SUFFIX === "string") { + assertPathSuffix(NODE_API_PATH_SUFFIX); +} -export const stripPathSuffixOption = new Option( - "--strip-path-suffix", - "Don't append escaped relative path to the library names (entails one Node-API module per package)" -).default(NODE_API_MODULES_STRIP_PATH_SUFFIX === "true"); +export const pathSuffixOption = new Option( + "--path-suffix ", + "Controls how the path of the addon inside a package is transformed into a library name" +) + .choices(PATH_SUFFIX_CHOICES) + .default(NODE_API_PATH_SUFFIX || "strip"); diff --git a/packages/host/src/node/cli/program.ts b/packages/host/src/node/cli/program.ts index 963ca414..d0f0f63f 100644 --- a/packages/host/src/node/cli/program.ts +++ b/packages/host/src/node/cli/program.ts @@ -20,7 +20,7 @@ import { } from "../path-utils"; import { command as vendorHermes } from "./hermes"; -import { stripPathSuffixOption } from "./options"; +import { pathSuffixOption } from "./options"; import { linkModules, pruneLinkedModules, ModuleLinker } from "./link-modules"; import { linkXcframework } from "./apple"; import { linkAndroidDir } from "./android"; @@ -67,123 +67,107 @@ program ) .option("--android", "Link Android modules") .option("--apple", "Link Apple modules") - .addOption(stripPathSuffixOption) - .action( - async (pathArg, { force, prune, stripPathSuffix, android, apple }) => { - console.log("Auto-linking Node-API modules from", chalk.dim(pathArg)); - - if (stripPathSuffix) { - console.log( - chalk.yellowBright("Warning:"), - "Stripping path suffixes, which might lead to name collisions" - ); - } - const platforms: PlatformName[] = []; - if (android) { - platforms.push("android"); - } - if (apple) { - platforms.push("apple"); - } + .addOption(pathSuffixOption) + .action(async (pathArg, { force, prune, pathSuffix, android, apple }) => { + console.log("Auto-linking Node-API modules from", chalk.dim(pathArg)); + const platforms: PlatformName[] = []; + if (android) { + platforms.push("android"); + } + if (apple) { + platforms.push("apple"); + } - if (platforms.length === 0) { - console.error( - `No platform specified, pass one or more of:`, - ...PLATFORMS.map((platform) => chalk.bold(`\n --${platform}`)) - ); - process.exitCode = 1; - return; - } + if (platforms.length === 0) { + console.error( + `No platform specified, pass one or more of:`, + ...PLATFORMS.map((platform) => chalk.bold(`\n --${platform}`)) + ); + process.exitCode = 1; + return; + } - for (const platform of platforms) { - const platformDisplayName = getPlatformDisplayName(platform); - const platformOutputPath = getAutolinkPath(platform); - const modules = await oraPromise( - () => - linkModules({ - platform, - fromPath: path.resolve(pathArg), - incremental: !force, - naming: { stripPathSuffix }, - linker: getLinker(platform), - }), - { - text: `Linking ${platformDisplayName} Node-API modules into ${prettyPath( - platformOutputPath - )}`, - successText: `Linked ${platformDisplayName} Node-API modules into ${prettyPath( + for (const platform of platforms) { + const platformDisplayName = getPlatformDisplayName(platform); + const platformOutputPath = getAutolinkPath(platform); + const modules = await oraPromise( + () => + linkModules({ + platform, + fromPath: path.resolve(pathArg), + incremental: !force, + naming: { pathSuffix }, + linker: getLinker(platform), + }), + { + text: `Linking ${platformDisplayName} Node-API modules into ${prettyPath( + platformOutputPath + )}`, + successText: `Linked ${platformDisplayName} Node-API modules into ${prettyPath( + platformOutputPath + )}`, + failText: (error) => + `Failed to link ${platformDisplayName} Node-API modules into ${prettyPath( platformOutputPath - )}`, - failText: (error) => - `Failed to link ${platformDisplayName} Node-API modules into ${prettyPath( - platformOutputPath - )}: ${error.message}`, - } - ); - - if (modules.length === 0) { - console.log("Found no Node-API modules 🤷"); + )}: ${error.message}`, } + ); - const failures = modules.filter((result) => "failure" in result); - const linked = modules.filter((result) => "outputPath" in result); - - for (const { originalPath, outputPath, skipped } of linked) { - const prettyOutputPath = outputPath - ? "→ " + prettyPath(path.basename(outputPath)) - : ""; - if (skipped) { - console.log( - chalk.greenBright("-"), - "Skipped", - prettyPath(originalPath), - prettyOutputPath, - "(up to date)" - ); - } else { - console.log( - chalk.greenBright("⚭"), - "Linked", - prettyPath(originalPath), - prettyOutputPath - ); - } - } + if (modules.length === 0) { + console.log("Found no Node-API modules 🤷"); + } - for (const { originalPath, failure } of failures) { - assert(failure instanceof SpawnFailure); - console.error( - "\n", - chalk.redBright("✖"), - "Failed to copy", - prettyPath(originalPath) + const failures = modules.filter((result) => "failure" in result); + const linked = modules.filter((result) => "outputPath" in result); + + for (const { originalPath, outputPath, skipped } of linked) { + const prettyOutputPath = outputPath + ? "→ " + prettyPath(path.basename(outputPath)) + : ""; + if (skipped) { + console.log( + chalk.greenBright("-"), + "Skipped", + prettyPath(originalPath), + prettyOutputPath, + "(up to date)" + ); + } else { + console.log( + chalk.greenBright("⚭"), + "Linked", + prettyPath(originalPath), + prettyOutputPath ); - console.error(failure.message); - failure.flushOutput("both"); - process.exitCode = 1; } + } - if (prune) { - await pruneLinkedModules(platform, modules); - } + for (const { originalPath, failure } of failures) { + assert(failure instanceof SpawnFailure); + console.error( + "\n", + chalk.redBright("✖"), + "Failed to copy", + prettyPath(originalPath) + ); + console.error(failure.message); + failure.flushOutput("both"); + process.exitCode = 1; + } + + if (prune) { + await pruneLinkedModules(platform, modules); } } - ); + }); program .command("list") .description("Lists Node-API modules") .argument("[from-path]", "Some path inside the app package", process.cwd()) .option("--json", "Output as JSON", false) - .addOption(stripPathSuffixOption) - .action(async (fromArg, { json, stripPathSuffix }) => { - if (stripPathSuffix) { - console.log( - chalk.yellowBright("Warning:"), - "Stripping path suffixes might lead to name collisions" - ); - } - + .addOption(pathSuffixOption) + .action(async (fromArg, { json, pathSuffix }) => { const rootPath = path.resolve(fromArg); const dependencies = findNodeApiModulePathsByDependency({ fromPath: rootPath, @@ -216,7 +200,7 @@ program ); logModulePaths( dependency.modulePaths.map((p) => path.join(dependency.path, p)), - { stripPathSuffix } + { pathSuffix } ); } } @@ -227,14 +211,14 @@ program .description( "Utility to print, module path, the hash of a single Android library" ) - .addOption(stripPathSuffixOption) - .action((pathInput, { stripPathSuffix }) => { + .addOption(pathSuffixOption) + .action((pathInput, { pathSuffix }) => { const resolvedModulePath = path.resolve(pathInput); const normalizedModulePath = normalizeModulePath(resolvedModulePath); const { packageName, relativePath } = determineModuleContext(resolvedModulePath); const libraryName = getLibraryName(resolvedModulePath, { - stripPathSuffix, + pathSuffix, }); console.log({ resolvedModulePath, diff --git a/packages/host/src/node/path-utils.test.ts b/packages/host/src/node/path-utils.test.ts index 96c44120..a5bc0bc0 100644 --- a/packages/host/src/node/path-utils.test.ts +++ b/packages/host/src/node/path-utils.test.ts @@ -30,7 +30,8 @@ function removeReadPermissions(p: string) { }; const result = fswin.setAttributesSync(p, attributes); - if (!result) throw new Error("can not set attributes to remove read permissions"); + if (!result) + throw new Error("can not set attributes to remove read permissions"); } function restoreReadPermissions(p: string) { @@ -47,7 +48,8 @@ function restoreReadPermissions(p: string) { }; const result = fswin.setAttributesSync(p, attributes); - if (!result) throw new Error("can not set attributes to restore read permissions"); + if (!result) + throw new Error("can not set attributes to restore read permissions"); } describe("isNodeApiModule", () => { @@ -61,20 +63,24 @@ describe("isNodeApiModule", () => { }); // there is no way to set ACLs on directories in Node.js on Windows with brittle powershell commands - it("returns false when directory cannot be read due to permissions", { skip: process.platform === "win32" }, (context) => { - const tempDirectoryPath = setupTempDirectory(context, { - "addon.android.node": "", - }); - removeReadPermissions(tempDirectoryPath); - try { - assert.equal( - isNodeApiModule(path.join(tempDirectoryPath, "addon")), - false - ); - } finally { - restoreReadPermissions(tempDirectoryPath); + it( + "returns false when directory cannot be read due to permissions", + { skip: process.platform === "win32" }, + (context) => { + const tempDirectoryPath = setupTempDirectory(context, { + "addon.android.node": "", + }); + removeReadPermissions(tempDirectoryPath); + try { + assert.equal( + isNodeApiModule(path.join(tempDirectoryPath, "addon")), + false + ); + } finally { + restoreReadPermissions(tempDirectoryPath); + } } - }); + ); it("throws when module file exists but is not readable", (context) => { const tempDirectoryPath = setupTempDirectory(context, { @@ -201,20 +207,42 @@ describe("getLibraryName", () => { }); assert.equal( getLibraryName(path.join(tempDirectoryPath, "addon"), { - stripPathSuffix: false, + pathSuffix: "keep", }), "my-package--addon" ); assert.equal( getLibraryName(path.join(tempDirectoryPath, "sub-directory/addon"), { - stripPathSuffix: false, + pathSuffix: "keep", }), "my-package--sub-directory-addon" ); }); - it("works when stripping relative path", (context) => { + it("strips path suffix", (context) => { + const tempDirectoryPath = setupTempDirectory(context, { + "package.json": `{ "name": "my-package" }`, + "addon.apple.node/addon.node": "// This is supposed to be a binary file", + "sub-directory/addon.apple.node/addon.node": + "// This is supposed to be a binary file", + }); + assert.equal( + getLibraryName(path.join(tempDirectoryPath, "addon"), { + pathSuffix: "strip", + }), + "my-package--addon" + ); + + assert.equal( + getLibraryName(path.join(tempDirectoryPath, "sub-directory", "addon"), { + pathSuffix: "strip", + }), + "my-package--addon" + ); + }); + + it("omits path suffix", (context) => { const tempDirectoryPath = setupTempDirectory(context, { "package.json": `{ "name": "my-package" }`, "addon.apple.node/addon.node": "// This is supposed to be a binary file", @@ -223,14 +251,14 @@ describe("getLibraryName", () => { }); assert.equal( getLibraryName(path.join(tempDirectoryPath, "addon"), { - stripPathSuffix: true, + pathSuffix: "omit", }), "my-package" ); assert.equal( - getLibraryName(path.join(tempDirectoryPath, "sub-directory-addon"), { - stripPathSuffix: true, + getLibraryName(path.join(tempDirectoryPath, "sub-directory", "addon"), { + pathSuffix: "omit", }), "my-package" ); @@ -376,14 +404,14 @@ describe("determineModuleContext", () => { describe("findNodeAddonForBindings()", () => { const expectedPaths = { - "addon_1": "addon_1.node", - "addon_2": "build/Release/addon_2.node", - "addon_3": "build/Debug/addon_3.node", - "addon_4": "build/addon_4.node", - "addon_5": "out/Release/addon_5.node", - "addon_6": "out/Debug/addon_6.node", - "addon_7": "Release/addon_7.node", - "addon_8": "Debug/addon_8.node", + addon_1: "addon_1.node", + addon_2: "build/Release/addon_2.node", + addon_3: "build/Debug/addon_3.node", + addon_4: "build/addon_4.node", + addon_5: "out/Release/addon_5.node", + addon_6: "out/Debug/addon_6.node", + addon_7: "Release/addon_7.node", + addon_8: "Debug/addon_8.node", }; for (const [name, relPath] of Object.entries(expectedPaths)) { diff --git a/packages/host/src/node/path-utils.ts b/packages/host/src/node/path-utils.ts index 6b2d8ed4..bcbcf181 100644 --- a/packages/host/src/node/path-utils.ts +++ b/packages/host/src/node/path-utils.ts @@ -15,10 +15,36 @@ export const PLATFORM_EXTENSIONS = { android: ".android.node", apple: ".apple.node", } as const satisfies Record; + export type PlatformExtentions = (typeof PLATFORM_EXTENSIONS)[PlatformName]; +export const PATH_SUFFIX_CHOICES = ["strip", "keep", "omit"] as const; +export type PathSuffixChoice = (typeof PATH_SUFFIX_CHOICES)[number]; + +export function assertPathSuffix( + value: unknown +): asserts value is PathSuffixChoice { + assert( + typeof value === "string", + `Expected a string, got ${typeof value} (${value})` + ); + assert( + (PATH_SUFFIX_CHOICES as readonly string[]).includes(value), + `Expected one of ${PATH_SUFFIX_CHOICES.join(", ")}` + ); +} + export type NamingStrategy = { - stripPathSuffix: boolean; + /** + * 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"`: 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: PathSuffixChoice; }; // Cache mapping package directory to package name across calls @@ -34,7 +60,9 @@ export function isNodeApiModule(modulePath: string): boolean { { // HACK: Take a shortcut (if applicable): existing `.node` files are addons try { - fs.accessSync(modulePath.endsWith(".node") ? modulePath : `${modulePath}.node`); + fs.accessSync( + modulePath.endsWith(".node") ? modulePath : `${modulePath}.node` + ); return true; } catch { // intentionally left empty @@ -158,9 +186,13 @@ export function escapePath(modulePath: string) { export function getLibraryName(modulePath: string, naming: NamingStrategy) { const { packageName, relativePath } = determineModuleContext(modulePath); const escapedPackageName = escapePath(packageName); - return naming.stripPathSuffix + return naming.pathSuffix === "omit" ? escapedPackageName - : `${escapedPackageName}--${escapePath(relativePath)}`; + : `${escapedPackageName}--${escapePath( + naming.pathSuffix === "strip" + ? path.basename(relativePath) + : relativePath + )}`; } export function prettyPath(p: string) {