Implementation of napi_fatal_error and napi_get*_version functions
#193
Conversation
🦋 Changeset detectedLatest commit: f523909 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
paradowstack
changed the title
napi_fatal_error and napi_get*_version(s) functionsnapi_fatal_error and napi_get*_version functions
There was a problem hiding this comment.
Pull Request Overview
This PR implements new Node-API functions and enhances the logging system to support multiple log levels. The changes add fatal error handling and version querying capabilities to the Node-API implementation, while refactoring the logging infrastructure to differentiate between debug, warning, and error messages.
- Added
napi_fatal_error,napi_get_node_version, andnapi_get_versionNode-API functions - Refactored logging system to support Debug, Warning, and Error log levels with unified internal implementation
- Created version constants header to centralize Node.js and Node-API version definitions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| generate-weak-node-api-injector.ts | Added new Node-API functions to the implemented runtime functions list |
| Versions.hpp | New header defining Node.js version constants and NAPI_VERSION |
| RuntimeNodeApi.hpp | Added function declarations for fatal error and version query functions |
| RuntimeNodeApi.cpp | Implemented the new Node-API functions with proper error handling |
| Logger.hpp | Extended logger interface with warning and error logging functions |
| Logger.cpp | Refactored logging implementation to support multiple log levels with unified internal logic |
paradowstack
force-pushed
the
kp/general-runtime-functions
branch
from
e6cd4ae to
eb36b61
Compare
paradowstack
added
Apple 🍎
paradowstack
force-pushed
the
kp/general-runtime-functions
branch
from
eb36b61 to
966b23a
Compare
packages/host/cpp/RuntimeNodeApi.cpp
Outdated
| size_t location_len, | ||
| const char* message, | ||
| size_t message_len) { | ||
| log_error("Fatal error in Node-API: %.*s: %.*s", |
There was a problem hiding this comment.
Perhaps put the message first and the location in a parenthesis afterwards? Just to get to the meat of the issue first.
| log_error("Fatal error in Node-API: %.*s: %.*s", | |
| log_error("Fatal error in Node-API: %.*s (in %.*s)", |
paradowstack
force-pushed
the
kp/general-runtime-functions
branch
from
0d644c7 to
fc3a35b
Compare
| void log_message_internal(LogLevel level, const char* format, va_list args) { | ||
| #if defined(__ANDROID__) | ||
| __android_log_vprint(androidLogLevel(level), LOG_TAG, format, args); | ||
| #elif defined(__APPLE__) | ||
| // iOS or macOS | ||
| fprintf(stderr, "[NodeApiHost] "); | ||
| const auto level_str = levelToString(level); | ||
| fprintf(stderr, LineFormat, level_str.data()); | ||
| vfprintf(stderr, format, args); | ||
| fprintf(stderr, "\n"); | ||
| #else | ||
| // Fallback for other platforms | ||
| fprintf(stdout, "[NodeApiHost] "); | ||
| const auto level_str = levelToString(level); | ||
| fprintf(stdout, LineFormat, level_str.data()); | ||
| vfprintf(stdout, format, args); | ||
| fprintf(stdout, "\n"); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
This logs to the native side - totally a good starting point, but would it make sense in future to log to the JS console so that we see it in the Metro/Chrome logs?
There was a problem hiding this comment.
Overall, it makes sense, but is the appearance of native logs in the JS console something common? 🤔
There was a problem hiding this comment.
Hard to say because Node.js is the main precedent here and I imagine these logs show up in its logs.
Mainly, if the Node-API module logs a fatal error, it feels like it might be good to be able to see it without having to attach Xcode/LLDB/Android Studio to the process.
There was a problem hiding this comment.
@shirakaba I agree it would be great to make these more discoverable, although I expect them to be rare. I'm a bit concerned though, that it might not be possible because of the semantics of the fatal error / exception functions, which cannot return and must abort the process. There won't be an app to display the error nor a DevTools connection to transport it once the program leaves the function.
There was a problem hiding this comment.
Could we put a line to the JS console just before we abort the process, so it is the last visible line and gives the developer information what went wrong?
There was a problem hiding this comment.
I'm a bit unsure on the semantics which need to be upheld by this function. If we call into other Node-API functions, to propagate the log message to JS, we might potentially (an edge-case for sure) get into an endless recursion. And hinted above, even if we called into console.log as an example, it won't make it into the DevTools client because the network transporting it from the app is likely not blocking and the app will die before it gets to send the message.
That's my hunch - we could definitely experiment with it 👍
This pull request introduces enhancements to the logging system and adds new Node-API functions.
Logging Enhancements:
Debug,Warning,Error)Node-API Functionality:
napi_fatal_errorto log fatal errors and terminate the processnapi_get_node_versionandnapi_get_versionfunctions to provide versioning information for Node.js and Node-API