Skip to content

Commit bb51216

Browse files
committed
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
1 parent 0a29fbd commit bb51216

7 files changed

Lines changed: 78 additions & 22 deletions

File tree

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
"react-native-node-api": patch
3+
---
4+
5+
Fix the Babel plugin rewriting `require(...)` calls that resolve to a
6+
same-named `.js`/`.cjs`/`.mjs`/`.json` file sitting next to a Node-API
7+
prebuild. Node's own module resolution always picks the source file over a
8+
`.node` addon in that case, so the plugin now leaves those calls alone
9+
instead of rewriting them to `requireNodeAddon(...)`, which would have loaded
10+
the wrong module at runtime.
11+
12+
Also exports `escapeBundleIdentifier`, used internally to derive a
13+
framework's `CFBundleIdentifier`, so it can be reused to verify one against
14+
its expected value.

packages/host/src/node/babel-plugin/plugin.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ describe("plugin", () => {
129129
itTransforms("and does not touch required JS files", {
130130
files: {
131131
"package.json": `{ "name": "my-package" }`,
132-
// TODO: Add a ./my-addon.node to make this test complete
132+
"my-addon.apple.node/my-addon.node":
133+
"// This is supposed to be a binary file",
133134
"my-addon.js": "// Some JS file",
134135
"index.js": `
135136
const addon = require('./my-addon');

packages/host/src/node/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ export {
2020
createXCframework,
2121
createUniversalAppleLibrary,
2222
determineXCFrameworkFilename,
23+
escapeBundleIdentifier,
2324
} from "./prebuilds/apple.js";
2425

2526
export {

packages/host/src/node/path-utils.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,18 +59,37 @@ export type NamingStrategy = {
5959
// Cache mapping package directory to package name across calls
6060
const packageNameCache = new Map<string, string>();
6161

62+
/**
63+
* Extensions Node's own `require()` resolves before it would ever consider `.node` -
64+
* see https://nodejs.org/api/modules.html#file-modules. A colliding file always wins,
65+
* so a module path resolving to one of these isn't ours to rewrite.
66+
*/
67+
const JS_RESOLVABLE_EXTENSIONS = [".js", ".cjs", ".mjs", ".json"];
68+
6269
/**
6370
* @param modulePath Batch-scans the path to the module to check (must be extensionless or end in .node)
6471
* @returns True if a platform specific prebuild exists for the module path, warns on unreadable modules.
6572
* @throws If the parent directory cannot be read, or if a detected module is unreadable.
6673
* TODO: Consider checking for a specific platform extension.
6774
*/
6875
export function isNodeApiModule(modulePath: string): boolean {
76+
const hasExplicitNodeExtension = modulePath.endsWith(".node");
77+
if (!hasExplicitNodeExtension) {
78+
const dir = path.dirname(modulePath);
79+
const baseName = path.basename(modulePath);
80+
if (
81+
JS_RESOLVABLE_EXTENSIONS.some((extension) =>
82+
fs.existsSync(path.join(dir, baseName + extension)),
83+
)
84+
) {
85+
return false;
86+
}
87+
}
6988
{
7089
// HACK: Take a shortcut (if applicable): existing `.node` files are addons
7190
try {
7291
fs.accessSync(
73-
modulePath.endsWith(".node") ? modulePath : `${modulePath}.node`,
92+
hasExplicitNodeExtension ? modulePath : `${modulePath}.node`,
7493
);
7594
return true;
7695
} catch {

packages/node-addon-examples/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
"bootstrap": "node --run copy-and-build"
3131
},
3232
"devDependencies": {
33+
"@expo/plist": "0.4.7",
3334
"cmake-rn": "workspace:*",
3435
"node-addon-examples": "github:nodejs/node-addon-examples#4b7dd86a85644610e6de80154df9acac9329b509",
3536
"gyp-to-cmake": "workspace:*",

packages/node-addon-examples/scripts/verify-prebuilds.mts

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,15 @@ import fs from "node:fs";
22
import assert from "node:assert/strict";
33
import path from "node:path";
44

5+
import plistPackage from "@expo/plist";
6+
import { escapeBundleIdentifier } from "react-native-node-api";
7+
58
import { DIRS } from "./cmake-projects.mjs";
69

10+
// `@expo/plist` is CommonJS; under Node's ESM interop the default import lands
11+
// one level deeper than TS's `esModuleInterop` cjs-compiled callers see it.
12+
const plist = plistPackage.default;
13+
714
const EXPECTED_ANDROID_ARCHS = ["armeabi-v7a", "arm64-v8a", "x86_64", "x86"];
815

916
const EXPECTED_XCFRAMEWORK_PLATFORMS = [
@@ -37,6 +44,29 @@ async function verifyAndroidPrebuild(dirent: fs.Dirent) {
3744
}
3845
}
3946

47+
/**
48+
* Asserts an Info.plist matches what `writeFrameworkInfoPlist` (in
49+
* `packages/host/src/node/prebuilds/apple.ts`) writes for a framework named
50+
* `libraryName`, built without a custom `--apple-bundle-identifier`.
51+
*/
52+
async function verifyFrameworkInfoPlist(
53+
infoPlistPath: string,
54+
libraryName: string,
55+
) {
56+
const contents = await fs.promises.readFile(infoPlistPath, "utf8");
57+
const infoPlist = plist.parse(contents) as Record<string, unknown>;
58+
assert.equal(
59+
infoPlist.CFBundleExecutable,
60+
libraryName,
61+
`Unexpected CFBundleExecutable in ${infoPlistPath}`,
62+
);
63+
assert.equal(
64+
infoPlist.CFBundleIdentifier,
65+
escapeBundleIdentifier(`com.callstackincubator.node-api.${libraryName}`),
66+
`Unexpected CFBundleIdentifier in ${infoPlistPath}`,
67+
);
68+
}
69+
4070
async function verifyApplePrebuild(dirent: fs.Dirent) {
4171
console.log("Verifying Apple prebuild", dirent.name, "in", dirent.parentPath);
4272
for (const arch of EXPECTED_XCFRAMEWORK_PLATFORMS) {
@@ -50,6 +80,7 @@ async function verifyApplePrebuild(dirent: fs.Dirent) {
5080
);
5181
assert(file.name.endsWith(".framework"), "Expected framework directory");
5282
const frameworkDir = path.join(file.parentPath, file.name);
83+
const libraryName = path.basename(file.name, ".framework");
5384
for (const file of await fs.promises.readdir(frameworkDir, {
5485
withFileTypes: true,
5586
})) {
@@ -65,8 +96,10 @@ async function verifyApplePrebuild(dirent: fs.Dirent) {
6596
"Expected only directory and files in framework",
6697
);
6798
if (file.name === "Info.plist") {
68-
// TODO: Verify the contents of the Info.plist file
69-
continue;
99+
await verifyFrameworkInfoPlist(
100+
path.join(frameworkDir, file.name),
101+
libraryName,
102+
);
70103
} else {
71104
assert(
72105
!file.name.endsWith(".node"),

pnpm-lock.yaml

Lines changed: 5 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)