Skip to content

Commit a848741

Browse files
claudekraenhansen
authored andcommitted
Move .js/.node precedence fix into isNodeApiModule, dedupe plist checks
- Replace the Babel-transform-time require.resolve() guard with a check inside isNodeApiModule itself, so the fix lives in the shared utility (also used by findNodeAddonForBindings) instead of duplicating Node's module resolution algorithm via a second, independent code path that could diverge from what Metro actually resolves at runtime. - Verify the Info.plist contents with a zod schema instead of ad hoc "in" checks on an untyped object, matching how the rest of the repo validates untrusted structured data. - Reuse the exported escapeBundleIdentifier instead of re-deriving the bundle-identifier escaping regex inline in the verify script, so the two can't silently drift apart. Closes #424 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q1k6UQJPPaqKEKmnsRUatt
1 parent 2d4f74f commit a848741

7 files changed

Lines changed: 57 additions & 50 deletions

File tree

.changeset/calm-bears-resolve.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
"react-native-node-api": patch
33
---
44

5-
Preserve Node.js module resolution precedence when a JavaScript file and native addon share a basename.
5+
Preserve Node.js module resolution precedence when a JavaScript file and native addon share a basename: `require('./foo')` no longer gets rewritten to load a Node-API addon when a same-named `foo.js`/`.cjs`/`.mjs`/`.json` file exists alongside it, since that source file is what `require()` actually resolves to. An explicit `require('./foo.node')` is unaffected.

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import assert from "node:assert/strict";
2-
import { createRequire } from "node:module";
32
import path from "node:path";
43

54
import type { PluginObj, NodePath } from "@babel/core";
@@ -102,7 +101,6 @@ export function plugin(): PluginObj {
102101
}
103102
} else if (
104103
!path.isAbsolute(id) &&
105-
!resolvesToNonNodeModule(id, this.filename) &&
106104
isNodeApiModule(path.join(from, id))
107105
) {
108106
const relativePath = path.join(from, id);
@@ -116,11 +114,3 @@ export function plugin(): PluginObj {
116114
},
117115
};
118116
}
119-
120-
function resolvesToNonNodeModule(id: string, filename: string): boolean {
121-
try {
122-
return !createRequire(filename).resolve(id).endsWith(".node");
123-
} catch {
124-
return false;
125-
}
126-
}

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: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,25 @@ export type NamingStrategy = {
5959
// Cache mapping package directory to package name across calls
6060
const packageNameCache = new Map<string, string>();
6161

62+
// Extensions Node's own require() resolves before ever trying `.node`.
63+
const COLLIDING_SOURCE_EXTENSIONS = [".js", ".cjs", ".mjs", ".json"];
64+
6265
/**
6366
* @param modulePath Batch-scans the path to the module to check (must be extensionless or end in .node)
6467
* @returns True if a platform specific prebuild exists for the module path, warns on unreadable modules.
6568
* @throws If the parent directory cannot be read, or if a detected module is unreadable.
6669
* TODO: Consider checking for a specific platform extension.
6770
*/
6871
export function isNodeApiModule(modulePath: string): boolean {
72+
if (
73+
!modulePath.endsWith(".node") &&
74+
COLLIDING_SOURCE_EXTENSIONS.some((extension) =>
75+
fs.existsSync(modulePath + extension),
76+
)
77+
) {
78+
// An explicit require('./foo.node') has no such ambiguity to defer to.
79+
return false;
80+
}
6981
{
7082
// HACK: Take a shortcut (if applicable): existing `.node` files are addons
7183
try {

packages/node-addon-examples/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
},
3939
"dependencies": {
4040
"assert": "^2.1.0",
41-
"react-native-node-api": "workspace:*"
41+
"react-native-node-api": "workspace:*",
42+
"zod": "^4.1.11"
4243
}
4344
}

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

Lines changed: 36 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,21 @@ import assert from "node:assert/strict";
33
import path from "node:path";
44

55
import plistModule from "@expo/plist";
6+
import { escapeBundleIdentifier } from "react-native-node-api";
7+
import { z } from "zod";
68

79
import { EXAMPLES_DIR } from "./cmake-projects.mjs";
810

11+
// @expo/plist is CJS with an `export default`; under this genuine ESM
12+
// (.mts) module's interop, the default import binds to the whole
13+
// `module.exports`, nesting the real API one `.default` deeper.
14+
const plist = plistModule.default;
15+
16+
const FrameworkInfoPlistSchema = z.object({
17+
CFBundleExecutable: z.string(),
18+
CFBundleIdentifier: z.string(),
19+
});
20+
921
const EXPECTED_ANDROID_ARCHS = ["armeabi-v7a", "arm64-v8a", "x86_64", "x86"];
1022

1123
const EXPECTED_XCFRAMEWORK_PLATFORMS = [
@@ -39,6 +51,27 @@ async function verifyAndroidPrebuild(dirent: fs.Dirent) {
3951
}
4052
}
4153

54+
async function verifyFrameworkInfoPlist(
55+
infoPlistPath: string,
56+
libraryName: string,
57+
) {
58+
const contents = await fs.promises.readFile(infoPlistPath, "utf8");
59+
const parsed = FrameworkInfoPlistSchema.parse(plist.parse(contents));
60+
assert.equal(
61+
parsed.CFBundleExecutable,
62+
libraryName,
63+
`Unexpected CFBundleExecutable in ${infoPlistPath}`,
64+
);
65+
assert.equal(
66+
parsed.CFBundleIdentifier,
67+
// Mirrors the default writeFrameworkInfoPlist derives in
68+
// packages/host/src/node/prebuilds/apple.ts, since none of the
69+
// examples pass --apple-bundle-identifier.
70+
escapeBundleIdentifier(`com.callstackincubator.node-api.${libraryName}`),
71+
`Unexpected CFBundleIdentifier in ${infoPlistPath}`,
72+
);
73+
}
74+
4275
async function verifyApplePrebuild(dirent: fs.Dirent) {
4376
console.log("Verifying Apple prebuild", dirent.name, "in", dirent.parentPath);
4477
for (const arch of EXPECTED_XCFRAMEWORK_PLATFORMS) {
@@ -68,27 +101,10 @@ async function verifyApplePrebuild(dirent: fs.Dirent) {
68101
);
69102
if (file.name === "Info.plist") {
70103
const libraryName = path.basename(frameworkDir, ".framework");
71-
const infoPlist: unknown = plistModule.default.parse(
72-
await fs.promises.readFile(
73-
path.join(frameworkDir, file.name),
74-
"utf8",
75-
),
76-
);
77-
assert(
78-
typeof infoPlist === "object" && infoPlist !== null,
79-
"Expected Info.plist to contain a dictionary",
80-
);
81-
assert("CFBundleExecutable" in infoPlist);
82-
assert("CFBundleIdentifier" in infoPlist);
83-
assert.equal(infoPlist.CFBundleExecutable, libraryName);
84-
assert.equal(
85-
infoPlist.CFBundleIdentifier,
86-
`com.callstackincubator.node-api.${libraryName}`.replace(
87-
/[^A-Za-z0-9-.]/g,
88-
"-",
89-
),
104+
await verifyFrameworkInfoPlist(
105+
path.join(frameworkDir, file.name),
106+
libraryName,
90107
);
91-
continue;
92108
} else {
93109
assert(
94110
!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)