From bb51216cda5d5ed49be88da0e749ed8146917004 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 07:20:27 +0000 Subject: [PATCH] Close two test gaps: unverified prebuild Info.plist and a missing .node fixture verify-prebuilds.mts now parses each Apple framework's Info.plist and asserts CFBundleExecutable/CFBundleIdentifier match what writeFrameworkInfoPlist wrote, instead of skipping the file. The Babel plugin test for "does not touch required JS files" now includes a sibling my-addon.apple.node next to my-addon.js, so the assertion is exercised rather than vacuously true. That exposed a real bug: isNodeApiModule didn't check whether a same-named .js/.cjs/.mjs/.json file would already satisfy the require() before ever considering a .node prebuild, so the plugin could rewrite a require() call that Node's own resolution would never route to the addon. Fixed to defer to a colliding source file, matching Node's own module resolution order. Closes #424 --- .changeset/babel-plugin-js-precedence.md | 14 +++++++ .../host/src/node/babel-plugin/plugin.test.ts | 3 +- packages/host/src/node/index.ts | 1 + packages/host/src/node/path-utils.ts | 21 ++++++++++- packages/node-addon-examples/package.json | 1 + .../scripts/verify-prebuilds.mts | 37 ++++++++++++++++++- pnpm-lock.yaml | 23 +++--------- 7 files changed, 78 insertions(+), 22 deletions(-) create mode 100644 .changeset/babel-plugin-js-precedence.md diff --git a/.changeset/babel-plugin-js-precedence.md b/.changeset/babel-plugin-js-precedence.md new file mode 100644 index 00000000..5a328ad7 --- /dev/null +++ b/.changeset/babel-plugin-js-precedence.md @@ -0,0 +1,14 @@ +--- +"react-native-node-api": patch +--- + +Fix the Babel plugin rewriting `require(...)` calls that resolve to a +same-named `.js`/`.cjs`/`.mjs`/`.json` file sitting next to a Node-API +prebuild. Node's own module resolution always picks the source file over a +`.node` addon in that case, so the plugin now leaves those calls alone +instead of rewriting them to `requireNodeAddon(...)`, which would have loaded +the wrong module at runtime. + +Also exports `escapeBundleIdentifier`, used internally to derive a +framework's `CFBundleIdentifier`, so it can be reused to verify one against +its expected value. diff --git a/packages/host/src/node/babel-plugin/plugin.test.ts b/packages/host/src/node/babel-plugin/plugin.test.ts index 7dae2979..4ea31bc7 100644 --- a/packages/host/src/node/babel-plugin/plugin.test.ts +++ b/packages/host/src/node/babel-plugin/plugin.test.ts @@ -129,7 +129,8 @@ describe("plugin", () => { 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.apple.node/my-addon.node": + "// This is supposed to be a binary file", "my-addon.js": "// Some JS file", "index.js": ` const addon = require('./my-addon'); diff --git a/packages/host/src/node/index.ts b/packages/host/src/node/index.ts index 1c4c69a9..baee642c 100644 --- a/packages/host/src/node/index.ts +++ b/packages/host/src/node/index.ts @@ -20,6 +20,7 @@ export { createXCframework, createUniversalAppleLibrary, determineXCFrameworkFilename, + escapeBundleIdentifier, } from "./prebuilds/apple.js"; export { diff --git a/packages/host/src/node/path-utils.ts b/packages/host/src/node/path-utils.ts index 0cb4506a..b4dda7ea 100644 --- a/packages/host/src/node/path-utils.ts +++ b/packages/host/src/node/path-utils.ts @@ -59,6 +59,13 @@ export type NamingStrategy = { // Cache mapping package directory to package name across calls const packageNameCache = new Map(); +/** + * Extensions Node's own `require()` resolves before it would ever consider `.node` - + * see https://nodejs.org/api/modules.html#file-modules. A colliding file always wins, + * so a module path resolving to one of these isn't ours to rewrite. + */ +const JS_RESOLVABLE_EXTENSIONS = [".js", ".cjs", ".mjs", ".json"]; + /** * @param modulePath Batch-scans the path to the module to check (must be extensionless or end in .node) * @returns True if a platform specific prebuild exists for the module path, warns on unreadable modules. @@ -66,11 +73,23 @@ const packageNameCache = new Map(); * TODO: Consider checking for a specific platform extension. */ export function isNodeApiModule(modulePath: string): boolean { + const hasExplicitNodeExtension = modulePath.endsWith(".node"); + if (!hasExplicitNodeExtension) { + const dir = path.dirname(modulePath); + const baseName = path.basename(modulePath); + if ( + JS_RESOLVABLE_EXTENSIONS.some((extension) => + fs.existsSync(path.join(dir, baseName + extension)), + ) + ) { + return false; + } + } { // HACK: Take a shortcut (if applicable): existing `.node` files are addons try { fs.accessSync( - modulePath.endsWith(".node") ? modulePath : `${modulePath}.node`, + hasExplicitNodeExtension ? modulePath : `${modulePath}.node`, ); return true; } catch { diff --git a/packages/node-addon-examples/package.json b/packages/node-addon-examples/package.json index acfd70ec..47e9ff76 100644 --- a/packages/node-addon-examples/package.json +++ b/packages/node-addon-examples/package.json @@ -30,6 +30,7 @@ "bootstrap": "node --run copy-and-build" }, "devDependencies": { + "@expo/plist": "0.4.7", "cmake-rn": "workspace:*", "node-addon-examples": "github:nodejs/node-addon-examples#4b7dd86a85644610e6de80154df9acac9329b509", "gyp-to-cmake": "workspace:*", diff --git a/packages/node-addon-examples/scripts/verify-prebuilds.mts b/packages/node-addon-examples/scripts/verify-prebuilds.mts index 94b4e1bb..cdb2e4a7 100644 --- a/packages/node-addon-examples/scripts/verify-prebuilds.mts +++ b/packages/node-addon-examples/scripts/verify-prebuilds.mts @@ -2,8 +2,15 @@ import fs from "node:fs"; import assert from "node:assert/strict"; import path from "node:path"; +import plistPackage from "@expo/plist"; +import { escapeBundleIdentifier } from "react-native-node-api"; + import { DIRS } from "./cmake-projects.mjs"; +// `@expo/plist` is CommonJS; under Node's ESM interop the default import lands +// one level deeper than TS's `esModuleInterop` cjs-compiled callers see it. +const plist = plistPackage.default; + const EXPECTED_ANDROID_ARCHS = ["armeabi-v7a", "arm64-v8a", "x86_64", "x86"]; const EXPECTED_XCFRAMEWORK_PLATFORMS = [ @@ -37,6 +44,29 @@ async function verifyAndroidPrebuild(dirent: fs.Dirent) { } } +/** + * Asserts an Info.plist matches what `writeFrameworkInfoPlist` (in + * `packages/host/src/node/prebuilds/apple.ts`) writes for a framework named + * `libraryName`, built without a custom `--apple-bundle-identifier`. + */ +async function verifyFrameworkInfoPlist( + infoPlistPath: string, + libraryName: string, +) { + const contents = await fs.promises.readFile(infoPlistPath, "utf8"); + const infoPlist = plist.parse(contents) as Record; + assert.equal( + infoPlist.CFBundleExecutable, + libraryName, + `Unexpected CFBundleExecutable in ${infoPlistPath}`, + ); + assert.equal( + infoPlist.CFBundleIdentifier, + escapeBundleIdentifier(`com.callstackincubator.node-api.${libraryName}`), + `Unexpected CFBundleIdentifier in ${infoPlistPath}`, + ); +} + async function verifyApplePrebuild(dirent: fs.Dirent) { console.log("Verifying Apple prebuild", dirent.name, "in", dirent.parentPath); for (const arch of EXPECTED_XCFRAMEWORK_PLATFORMS) { @@ -50,6 +80,7 @@ async function verifyApplePrebuild(dirent: fs.Dirent) { ); assert(file.name.endsWith(".framework"), "Expected framework directory"); const frameworkDir = path.join(file.parentPath, file.name); + const libraryName = path.basename(file.name, ".framework"); for (const file of await fs.promises.readdir(frameworkDir, { withFileTypes: true, })) { @@ -65,8 +96,10 @@ async function verifyApplePrebuild(dirent: fs.Dirent) { "Expected only directory and files in framework", ); if (file.name === "Info.plist") { - // TODO: Verify the contents of the Info.plist file - continue; + await verifyFrameworkInfoPlist( + path.join(frameworkDir, file.name), + libraryName, + ); } else { assert( !file.name.endsWith(".node"), diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6a99748a..07b15d57 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -281,6 +281,9 @@ importers: specifier: workspace:* version: link:../host devDependencies: + '@expo/plist': + specifier: 0.4.7 + version: 0.4.7 cmake-rn: specifier: workspace:* version: link:../cmake-rn @@ -4095,14 +4098,6 @@ packages: resolution: {integrity: sha512-RvwwcruNjI1ncT5xRakeyS9Lf8lcItv34KD+aif+VH9kduAyfYBipGh12274xtenIPZ119/R9BdTBa8gAwSh0A==} engines: {node: '>=12'} - pify@4.0.1: - resolution: {integrity: sha512-uB80kBFb/tfd68bVleG9T5GGsGPjJrLAUpR5PZIrhBnIaRTQRjqdJSsIKkOP6OAIFbj7GOrcudc5pNjZ+geV2g==} - engines: {node: '>=6'} - - pirates@4.0.7: - resolution: {integrity: sha512-TfySrs/5nm8fQJDcBDuUng3VOUKsd7S+zqvbOTiGXHfxX4wK31ard+hoNuvkicM/2YFzlpDgABOevKSsB4G/FA==} - engines: {node: '>= 6'} - pkg-dir@8.0.0: resolution: {integrity: sha512-4peoBq4Wks0riS0z8741NVv+/8IiTvqnZAr8QGgtdifrtpdXbNw/FxRS1l6NFqm4EMzuS0EDqNNx4XGaz8cuyQ==} engines: {node: '>=18'} @@ -4412,10 +4407,6 @@ packages: sisteransi@1.0.5: resolution: {integrity: sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg==} - slash@3.0.0: - resolution: {integrity: sha512-g9Q1haeby36OSStwb4ntCGGGaKsaVSjQ68fBxoQcutl5fS1vuY18H3wSt3jFyFtrkx+Kz0V1G85A4MyAdDMi2Q==} - engines: {node: '>=8'} - slice-ansi@2.1.0: resolution: {integrity: sha512-Qu+VC3EwYLldKa1fCxuuvULvSJOKEgk9pi8dZeCVK7TqBfUNTH4sFkk4joj8afVSfAYgJoSOetjx9QWOJ5mYoQ==} engines: {node: '>=6'} @@ -6790,7 +6781,9 @@ snapshots: metro-runtime: 0.87.0 transitivePeerDependencies: - '@babel/core' + - bufferutil - supports-color + - utf-8-validate '@react-native/normalize-colors@0.88.0-nightly-20260809-db662caea': {} @@ -8994,10 +8987,6 @@ snapshots: picomatch@4.0.5: {} - pify@4.0.1: {} - - pirates@4.0.7: {} - pkg-dir@8.0.0: dependencies: find-up-simple: 1.0.1 @@ -9378,8 +9367,6 @@ snapshots: sisteransi@1.0.5: {} - slash@3.0.0: {} - slice-ansi@2.1.0: dependencies: ansi-styles: 3.2.1