Skip to content

Drop the host's shadowing implementations of runtime Node-API functions - #434

Merged
kraenhansen merged 2 commits into
nextfrom
claude/issue-428-drop-shadowing-napi-shims
Aug 13, 2026
Merged

Drop the host's shadowing implementations of runtime Node-API functions#434
kraenhansen merged 2 commits into
nextfrom
claude/issue-428-drop-shadowing-napi-shims

Conversation

@kraenhansen

Copy link
Copy Markdown
Collaborator

Summary

Hermes' first-party Node-API (adopted in #372, wired up via hermes_napi_host in #398) already implements the entire runtime surface, including buffers, napi_fatal_error, napi_get_version and napi_get_node_version. But packages/host/cpp/RuntimeNodeApi.{cpp,hpp} still defined eight of these — and won, because the generated injector (packages/host/scripts/generate-injector.mts) resolves each NodeApiHost field by unqualified name inside namespace callstack::react_native_node_api, and RuntimeNodeApi.hpp is included there. So unqualified lookup found the host's shim before it ever reached Hermes' exported symbol.

This PR removes seven of those eight functions, letting the unqualified-name lookup fall through to Hermes' own implementations (the same mechanism every other Node-API function in this codebase already relies on — RuntimeNodeApi.cpp itself calls napi_create_arraybuffer, napi_typeof, etc. without defining them, and those already resolve to Hermes today):

  • napi_create_buffer, napi_create_buffer_copy, napi_create_external_buffer, napi_get_buffer_info, napi_is_buffer — dropped.
  • napi_get_version — dropped (Hermes' answer is identical, NAPI_VERSION).
  • napi_get_node_version — dropped. Closes Implement napi_get_node_version #67: addons now get Hermes' actual version instead of napi_generic_failure.
  • napi_fatal_errorkept, with a comment on the declaration explaining why: Hermes routes it to stderr, which isn't logcat on Android, while the host's version reaches logcat via the NodeApiHost logger tag.

This also removes two bugs the shims carried:

  1. napi_get_buffer_info overwrote a mutable global (ArrayType) used by napi_create_buffer/napi_create_external_buffer; calling it on a non-Uint8Array typed array (e.g. Float64Array) corrupted every later buffer creation on that process, and raced across runtimes.
  2. napi_create_buffer_copy accepted result_data but never wrote it.

napi_is_buffer/napi_get_buffer_info also become stricter, matching Node: true/napi_ok only for Uint8Array, napi_invalid_arg otherwise (previously any ArrayBuffer/TypedArray). This is an intentional, documented behavior change (see the changeset), and also tightens #171.

RuntimeNodeApi.{cpp,hpp} keep their own translation unit rather than folding into Logger-adjacent code — the file now holds exactly the one shim the host deliberately keeps shadowing, and renaming would touch the generated injector, CMakeLists.txt, and the podspec's glob for no functional benefit. packages/host/android/CMakeLists.txt needed no changes as a result.

A changeset (patch) documents the observable behavior change for napi_get_node_version and the buffer functions.

Why this is safe (reachability of Hermes' symbols)

I verified, source-level, that removing the host's declarations doesn't strand these calls:

  • The generated injector's NodeApiHost { .name = name, ... } entries are produced for every Node-API function from node-api-headers (packages/weak-node-api/src/node-api-functions.ts), regardless of whether the host defines it — this list is unaffected by this change.
  • RuntimeNodeApi.cpp already calls many Node-API functions (napi_create_arraybuffer, napi_typeof, napi_is_arraybuffer, napi_get_arraybuffer_info, napi_get_typedarray_info, napi_create_external_arraybuffer) without defining them anywhere in this repo — those already resolve, at link time, to Hermes' exported symbols today. Once RuntimeNodeApi.hpp no longer declares the seven removed functions inside namespace callstack::react_native_node_api, unqualified lookup for them falls through to the same global-namespace declarations from node_api.h (already used identically for the calls above) and links against Hermes' hermesvm/xcframework the same way.

Verified

  • pnpm install && pnpm run build — passes.
  • pnpm --filter react-native-node-api run test — passes except three pre-existing, unrelated failures in path-utils.test.ts around unreadable-file/permission handling; confirmed those fail identically on unmodified origin/next in this sandbox (it runs as root, which bypasses the permission checks the tests rely on).
  • pnpm exec eslint packages/host and pnpm exec prettier --check on touched files — clean.
  • Checked no other files in the repo reference the removed functions or RuntimeNodeApi symbols outside the two edited files.

Not verified (needs a real device/toolchain)

  • Native C++ compilation — no Android/iOS toolchain is available in this sandbox. The change was reviewed at the source level only.
  • packages/node-addon-examples/tests/buffers still passing on device — reviewed the test source (addon.c/addon.js) against both the old and new (Hermes) semantics; every buffer the test creates goes through napi_create_buffer/napi_create_external_buffer (producing real Uint8Arrays under both implementations), and its one non-buffer check (invalidObjectAsBuffer) passes NULL for both data/length, which trips napi_invalid_arg under either implementation. So this should still pass, but this checklist item from Drop the host's shadowing implementations of runtime Node-API functions #428 remains pending human/CI verification on an actual device.

Closes #428, closes #67.


🤖 Generated with Claude Code

https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm


Generated by Claude Code

@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) host labels Aug 13, 2026 — with Claude
@kraenhansen kraenhansen added Host 🏡 Our `react-native-node-api-modules` package and removed host labels Aug 13, 2026
kraenhansen pushed a commit that referenced this pull request Aug 13, 2026
The label table and example condition used plain "host", copied from
.github/workflows/check.yml's host-cpp-tests job at the time. That job's
condition was itself wrong — the repository's real label is "Host 🏡" (see
issues #428/#420/#412) — confirmed and fixed on #434's branch. Update this
doc to match, and note that a label condition needs to be checked against
the real, existing label rather than trusted at face value.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm

Copy link
Copy Markdown
Collaborator Author

Swapped the host label (deleted) for Host 🏡, and pushed a fix for a real bug this surfaced: host-cpp-tests in .github/workflows/check.yml was checking for a label literally named host, which never matched the actual Host 🏡 label used everywhere else in the repo — so that job has only ever run on pushes to main/next, never via a labeled PR, until now.


🤖 Generated with Claude Code

https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm


Generated by Claude Code

@kraenhansen kraenhansen self-assigned this Aug 13, 2026
kraenhansen added a commit that referenced this pull request Aug 13, 2026
* docs: instruct Claude Code to attach CI labels when opening PRs

.github/workflows/check.yml's pull_request trigger fires only on opened,
synchronize and reopened — not labeled. Several jobs are gated on labels
being present in that triggering event's payload (host, Apple 🍎,
Android 🤖, MacOS 💻, Ferric 🦀, weak-node-api), so a label attached after
PR creation (the only way the create_pull_request MCP tool allows, since it
has no labels parameter) never actually triggers those jobs without a
follow-up push.

Adds .claude/CLAUDE.md documenting the label -> job mapping and the
attach-then-push-again workflow, prompted by PR #434 needing labels added
and retriggered after the fact.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm

* docs: correct the host label name to "Host 🏡"

The label table and example condition used plain "host", copied from
.github/workflows/check.yml's host-cpp-tests job at the time. That job's
condition was itself wrong — the repository's real label is "Host 🏡" (see
issues #428/#420/#412) — confirmed and fixed on #434's branch. Update this
doc to match, and note that a label condition needs to be checked against
the real, existing label rather than trusted at face value.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm

---------

Co-authored-by: Claude <noreply@anthropic.com>
claude added 2 commits August 13, 2026 10:32
Hermes' first-party Node-API (adopted in #372, integrated via
hermes_napi_host in #398) already implements the buffer functions,
napi_get_version and napi_get_node_version. RuntimeNodeApi.{cpp,hpp} still
defined all of these, and since the generated injector
(scripts/generate-injector.mts) resolves each NodeApiHost field by
unqualified name inside `namespace callstack::react_native_node_api`, the
host's shims won and Hermes' implementations were never reached.

Remove napi_create_buffer, napi_create_buffer_copy,
napi_create_external_buffer, napi_get_buffer_info, napi_is_buffer,
napi_get_version and napi_get_node_version from RuntimeNodeApi.{cpp,hpp},
letting unqualified lookup fall through to Hermes' own symbols. This also
fixes two bugs the shims carried:

- napi_get_buffer_info wrote its typed-array-kind output into a mutable
  global (`ArrayType`) that every subsequent napi_create_buffer /
  napi_create_external_buffer call read back, so calling it on e.g. a
  Float64Array corrupted every later buffer creation (and raced across
  runtimes).
- napi_create_buffer_copy accepted `result_data` but never wrote it.

napi_is_buffer / napi_get_buffer_info also become stricter, matching Node:
true/napi_ok only for Uint8Array, napi_invalid_arg otherwise, instead of
accepting any ArrayBuffer/TypedArray.

Keep napi_fatal_error's host-side implementation: Hermes routes it to
stderr, which is not logcat on Android, while the host's version reaches
logcat via the "NodeApiHost" logger tag. Documented why this one
intentionally keeps shadowing Hermes so a future sweep doesn't remove it
as dead weight.

RuntimeNodeApi.{cpp,hpp} keep their own translation unit rather than
folding into Logger-adjacent code: the file now holds exactly the one
shim the host deliberately keeps, and renaming would touch the injector,
CMakeLists and podspec globbing for no functional benefit.

Adds a changeset (patch) for the observable behavior change: addons now
see Hermes' real napi_get_node_version instead of napi_generic_failure,
and the stricter buffer type-checking. Closes #67.

Verified: pnpm install && pnpm run build, pnpm --filter react-native-node-api
run test (pre-existing failures only, confirmed present on unmodified
origin/next too - they stem from running as root, not this change),
eslint and prettier on touched files. Native C++ compilation was not
verified - no Android/iOS toolchain is available in this environment.

Closes #428

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm
The job checked for a label literally named "host", but the repository's
real label (used on issues, e.g. #428/#420/#412) is "Host 🏡" — a label
named plain "host" existed too, seemingly a leftover/duplicate, and has
since been deleted. The condition never actually matched the label anyone
would apply in practice, so this job only ever ran on pushes to main/next,
never on a labeled PR.

Found while attaching labels to this PR: the CI still showed green with
host-cpp-tests silently not running, exactly the kind of gap
.claude/CLAUDE.md (#436) exists to prevent.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DaK9eAAF5G8wj6UT8VekAm
@kraenhansen
kraenhansen force-pushed the claude/issue-428-drop-shadowing-napi-shims branch from 0317817 to 3a7323b Compare August 13, 2026 10:33
@kraenhansen
kraenhansen merged commit c22f39c into next Aug 13, 2026
18 checks passed
@kraenhansen
kraenhansen deleted the claude/issue-428-drop-shadowing-napi-shims branch August 13, 2026 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Host 🏡 Our `react-native-node-api-modules` package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drop the host's shadowing implementations of runtime Node-API functions Implement napi_get_node_version

2 participants