Skip to content

Verify prebuild metadata and JS addon precedence - #455

Closed
huytdps13400 wants to merge 1 commit into
callstackincubator:mainfrom
huytdps13400:fix/424-verify-prebuild-fixtures
Closed

Verify prebuild metadata and JS addon precedence#455
huytdps13400 wants to merge 1 commit into
callstackincubator:mainfrom
huytdps13400:fix/424-verify-prebuild-fixtures

Conversation

@huytdps13400

Copy link
Copy Markdown

Summary

  • add the missing sibling .node fixture to the JS require precedence test
  • preserve Node resolution when an extensionless require resolves to JS/JSON before a native addon
  • parse every generated framework Info.plist and verify CFBundleExecutable plus the expected escaped CFBundleIdentifier
  • add portable plist verifier unit tests to the root test gate
  • declare the existing plist parser directly in the package that uses it and add a patch changeset

The completed Babel fixture exposed a real bug: current code transformed require("./my-addon") into requireNodeAddon(...) even when my-addon.js existed. This PR fixes that precedence rather than weakening the new test.

Verification

  • RED: completed Babel fixture failed because it emitted requireNodeAddon
  • focused Babel plugin suite: 10/10
  • focused plist verifier suite: 3/3
  • pnpm run build
  • pnpm test: 88 tests passed (62 host, 20 gyp-to-cmake, 3 cmake-rn, 3 plist verifier)
  • ESLint on every changed TypeScript file
  • pnpm run prettier:check
  • pnpm run depcheck
  • pnpm run publint
  • frozen lockfile validation with Node 24 / pnpm 10.33

Full root lint additionally requires generated native typings from the Rust/clang-format bootstrap; those toolchains are not installed locally. The changed files are lint-clean, and CI runs the documented bootstrap before its root lint job.

Fixes #424

@kraenhansen kraenhansen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Compared this against the other open attempt at #424 (#426) and against #450 (closed, but its rationale is directly relevant here). Verified locally by checking out both branches, building, and running the affected test files.

The precedence fix only covers one of the two call sites that need it. resolvesToNonAddon (packages/host/src/node/babel-plugin/plugin.ts:70-76) guards the plain require(id) branch at lines 111-115, but the require('bindings')(name) branch just above it (lines 99-109) calls findNodeAddonForBindings, which internally calls the unpatched isNodeApiModule in path-utils.ts — so a .js/.json file that shadows a same-named .node addon still gets misdetected as the addon when reached via bindings(). This is exactly why #450 (built on top of #426) moved the fix into the shared isNodeApiModule utility instead of guarding only the plugin's require() handling — same root cause, both callers fixed. Worth confirming with a bindings() fixture analogous to the one added to plugin.test.ts.

Fixing this via createRequire(...).resolve() at transform time is also the approach #450 deliberately avoided, for a reason worth weighing here too: this runs Node's own CommonJS resolution algorithm to predict what Metro will resolve at bundle time, but Metro has its own resolver (platform extensions, Haste, etc.) that can diverge from Node's. It's also fail-open — any exception from .resolve() (e.g. a platform-specific .ios.js that Node's resolver doesn't know how to find) is swallowed and falls back to the old (buggy) behavior. A direct sibling-file check in isNodeApiModule, as #426/#450 do, doesn't depend on that assumption holding.

Base branch: this targets main, while #424's other three attempts (#426, #448, #449, #450) all targeted next, which currently has real work main lacks. git merge-tree shows this produces actual conflicts against next (root package.json's build/copy-and-build/test scripts, and the DIRS-vs-EXAMPLES_DIR loop in verify-prebuilds.mts) — worth rebasing onto/retargeting next before this can land there.

One genuine improvement worth keeping regardless of outcome here: verify-prebuilds.test.mts unit-tests verifyFrameworkInfoPlist directly (3/3 passing locally) and wires it into the fast root pnpm test gate via test:verify, without requiring a full native build — #426 only exercises its Info.plist check through the full node-addon-examples build+verify flow. That's a nice piece to carry over into whichever PR ends up merged.


Generated by Claude Code

@huytdps13400

Copy link
Copy Markdown
Author

Confirmed the review against the current next branch, #426, and the closed consolidation attempt #450.

  • The precedence check belongs in shared isNodeApiModule, which covers both plain require() and require("bindings") through findNodeAddonForBindings.
  • Node-style extensionless precedence should cover .js and .json, while .cjs/.mjs do not shadow .node; fix: verify native prebuild fixtures #426 already implements and tests this.
  • createRequire().resolve() is not a reliable proxy for Metro resolution.
  • fix: verify native prebuild fixtures #426 already targets next, is mergeable, and has maintainer approval.

Keeping #455 active would duplicate the approved contribution, so I am closing it in favor of #426. The independent verifyFrameworkInfoPlist unit-test work remains available on commit 4bd6ab5e and can be proposed as a focused follow-up after #426 lands without competing with the existing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants