Follow-up from the #56 sweep (see #61, #62, #63, #66, #68, #65, #67).
Since we adopted Hermes' first-party Node-API (#372) and implemented hermes_napi_host (#398), Hermes implements the entire runtime surface itself — including buffers, napi_fatal_error, napi_get_version and napi_get_node_version. But packages/host/cpp/RuntimeNodeApi.{cpp,hpp} still defines eight of those functions, and they win: the generated injector resolves its struct initializers inside namespace callstack::react_native_node_api (packages/host/scripts/generate-injector.mts), and RuntimeNodeApi.hpp is included there, so unqualified lookup finds the host's definition and never reaches the symbol Hermes exports.
In other words the shims — written when Hermes had no Node-API of its own — are what addons call today, and they now diverge from the engine we delegate everything else to.
What shadows what
| Function |
Host (RuntimeNodeApi.cpp) |
Hermes (API/napi/) |
Suggestion |
napi_create_buffer |
ArrayBuffer + Uint8Array view |
hermes_napi_buffer.cpp, same shape |
drop |
napi_create_buffer_copy |
as above + memcpy, never writes result_data |
hermes_napi_buffer.cpp, writes it |
drop |
napi_create_external_buffer |
external ArrayBuffer + view |
hermes_napi_buffer.cpp |
drop |
napi_get_buffer_info |
accepts any ArrayBuffer/TypedArray, napi_ok + zeroes for anything else |
napi_invalid_arg for non-Uint8Array, like Node |
drop |
napi_is_buffer |
true for any ArrayBuffer/TypedArray |
true only for Uint8Array, like Node |
drop (also tightens #171) |
napi_get_version |
*result = NAPI_VERSION |
identical |
drop |
napi_get_node_version |
napi_generic_failure |
Hermes version, release name "hermes" |
drop — decided in #67 |
napi_fatal_error |
logs via the host logger, then abort() |
hermes_fatal → llvh::report_fatal_error → stderr |
keep, see below |
Two bugs the shims carry today
Both disappear with the shims, which is the main reason to do this rather than leave it as tidying:
ArrayType is a mutable global that napi_get_buffer_info overwrites. RuntimeNodeApi.cpp:6 declares auto ArrayType = napi_uint8_array; at global scope, and napi_get_buffer_info passes &ArrayType as the out type parameter of napi_get_typedarray_info (RuntimeNodeApi.cpp:98). Since the host's napi_is_buffer treats every typed array as a buffer, an addon calling napi_get_buffer_info on, say, a Float64Array leaves ArrayType == napi_float64_array — and every subsequent napi_create_buffer / napi_create_external_buffer then produces a Float64Array (with element counts read as byte lengths) instead of a Uint8Array. It is also a plain data race once more than one runtime is alive.
napi_create_buffer_copy ignores result_data. The parameter is accepted and never written (RuntimeNodeApi.cpp:25-41), so an addon that passes a non-NULL result_data — Node documents it as optional, i.e. skippable by passing NULL, not as ignorable by the implementation — reads back uninitialized memory. Hermes writes it.
Why napi_fatal_error should stay
Hermes routes it to hermes::hermes_fatal → llvh::report_fatal_error, which writes to stderr. On Android stderr is not logcat, so the message would be lost exactly when it matters most; the host's version goes through log_error and reaches logcat with the NodeApiHost tag (packages/host/cpp/Logger.cpp). Worth keeping the shim and commenting why it deliberately shadows Hermes', so the next sweep doesn't remove it as dead weight.
Suggested steps
Not filed as a sub-issue of #56, since that umbrella tracks implementing the functions rather than cleaning up after them — happy to attach it if you'd rather keep them together.
Follow-up from the #56 sweep (see #61, #62, #63, #66, #68, #65, #67).
Since we adopted Hermes' first-party Node-API (#372) and implemented
hermes_napi_host(#398), Hermes implements the entire runtime surface itself — including buffers,napi_fatal_error,napi_get_versionandnapi_get_node_version. Butpackages/host/cpp/RuntimeNodeApi.{cpp,hpp}still defines eight of those functions, and they win: the generated injector resolves its struct initializers insidenamespace callstack::react_native_node_api(packages/host/scripts/generate-injector.mts), andRuntimeNodeApi.hppis included there, so unqualified lookup finds the host's definition and never reaches the symbol Hermes exports.In other words the shims — written when Hermes had no Node-API of its own — are what addons call today, and they now diverge from the engine we delegate everything else to.
What shadows what
RuntimeNodeApi.cpp)API/napi/)napi_create_bufferArrayBuffer+Uint8Arrayviewhermes_napi_buffer.cpp, same shapenapi_create_buffer_copymemcpy, never writesresult_datahermes_napi_buffer.cpp, writes itnapi_create_external_bufferArrayBuffer+ viewhermes_napi_buffer.cppnapi_get_buffer_infoArrayBuffer/TypedArray,napi_ok+ zeroes for anything elsenapi_invalid_argfor non-Uint8Array, like Nodenapi_is_bufferArrayBuffer/TypedArrayUint8Array, like Nodenapi_get_version*result = NAPI_VERSIONnapi_get_node_versionnapi_generic_failure"hermes"napi_fatal_errorabort()hermes_fatal→llvh::report_fatal_error→ stderrTwo bugs the shims carry today
Both disappear with the shims, which is the main reason to do this rather than leave it as tidying:
ArrayTypeis a mutable global thatnapi_get_buffer_infooverwrites.RuntimeNodeApi.cpp:6declaresauto ArrayType = napi_uint8_array;at global scope, andnapi_get_buffer_infopasses&ArrayTypeas the out type parameter ofnapi_get_typedarray_info(RuntimeNodeApi.cpp:98). Since the host'snapi_is_buffertreats every typed array as a buffer, an addon callingnapi_get_buffer_infoon, say, aFloat64ArrayleavesArrayType == napi_float64_array— and every subsequentnapi_create_buffer/napi_create_external_bufferthen produces aFloat64Array(with element counts read as byte lengths) instead of aUint8Array. It is also a plain data race once more than one runtime is alive.napi_create_buffer_copyignoresresult_data. The parameter is accepted and never written (RuntimeNodeApi.cpp:25-41), so an addon that passes a non-NULLresult_data— Node documents it as optional, i.e. skippable by passingNULL, not as ignorable by the implementation — reads back uninitialized memory. Hermes writes it.Why
napi_fatal_errorshould stayHermes routes it to
hermes::hermes_fatal→llvh::report_fatal_error, which writes to stderr. On Android stderr is not logcat, so the message would be lost exactly when it matters most; the host's version goes throughlog_errorand reaches logcat with theNodeApiHosttag (packages/host/cpp/Logger.cpp). Worth keeping the shim and commenting why it deliberately shadows Hermes', so the next sweep doesn't remove it as dead weight.Suggested steps
napi_get_versionfromRuntimeNodeApi.{cpp,hpp}.napi_get_node_versionas well — decided in Implementnapi_get_node_version#67: Hermes answers with its own version instead of the currentnapi_generic_failure. Closes Implementnapi_get_node_version#67, and wants a changeset, since it is an observable behaviour change for addons.napi_fatal_error, with a comment explaining the shadowing is intentional (logcat).packages/node-addon-examples/tests/buffersstill passes on device — the stricternapi_is_buffer/napi_get_buffer_infoare a behaviour change, and the port of Node'stest_bufferis where that shows up.napi_fatal_errorgoes, consider whetherRuntimeNodeApi.{cpp,hpp}still earns its own file or folds intoLogger-adjacent code, and updatepackages/host/android/CMakeLists.txtaccordingly.Not filed as a sub-issue of #56, since that umbrella tracks implementing the functions rather than cleaning up after them — happy to attach it if you'd rather keep them together.