Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/kind-forks-talk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"react-native-node-api": patch
---

Added implementation of napi_fatal_error, napi_get_node_version and napi_get_version. Improved the Logger functionalities
68 changes: 59 additions & 9 deletions packages/host/cpp/Logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,77 @@
#include <TargetConditionals.h>
#endif

namespace callstack::nodeapihost {
void log_debug(const char *format, ...) {
// TODO: Disable logging in release builds
namespace {
constexpr auto LineFormat = "[%s] [NodeApiHost] ";

va_list args;
va_start(args, format);
enum class LogLevel { Debug, Warning, Error };

constexpr std::string_view levelToString(LogLevel level) {
switch (level) {
case LogLevel::Debug:
return "DEBUG";
case LogLevel::Warning:
return "WARNING";
case LogLevel::Error:
return "ERROR";
default:
return "UNKNOWN";
}
}

#if defined(__ANDROID__)
__android_log_vprint(ANDROID_LOG_DEBUG, LOG_TAG, format, args);
constexpr int androidLogLevel(LogLevel level) {
switch (level) {
case LogLevel::Debug:
return ANDROID_LOG_DEBUG;
case LogLevel::Warning:
return ANDROID_LOG_WARN;
case LogLevel::Error:
return ANDROID_LOG_ERROR;
default:
return ANDROID_LOG_UNKNOWN;
}
}
#endif

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
}
Comment on lines +45 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overall, it makes sense, but is the appearance of native logs in the JS console something common? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 👍

} // anonymous namespace

namespace callstack::nodeapihost {

void log_debug(const char* format, ...) {
// TODO: Disable logging in release builds
va_list args;
va_start(args, format);
log_message_internal(LogLevel::Debug, format, args);
va_end(args);
}
void log_warning(const char* format, ...) {
va_list args;
va_start(args, format);
log_message_internal(LogLevel::Warning, format, args);
va_end(args);
}
void log_error(const char* format, ...) {
va_list args;
va_start(args, format);
log_message_internal(LogLevel::Error, format, args);
va_end(args);
}
} // namespace callstack::nodeapihost
} // namespace callstack::nodeapihost
6 changes: 4 additions & 2 deletions packages/host/cpp/Logger.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,7 @@
#include <string>

namespace callstack::nodeapihost {
void log_debug(const char *format, ...);
}
void log_debug(const char* format, ...);
void log_warning(const char* format, ...);
void log_error(const char* format, ...);
} // namespace callstack::nodeapihost
38 changes: 38 additions & 0 deletions packages/host/cpp/RuntimeNodeApi.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "RuntimeNodeApi.hpp"
#include <string>
#include "Logger.hpp"
#include "Versions.hpp"

auto ArrayType = napi_uint8_array;

Expand Down Expand Up @@ -121,4 +123,40 @@ napi_status napi_create_external_buffer(napi_env env,
return napi_create_typedarray(env, ArrayType, length, buffer, 0, result);
}

void napi_fatal_error(const char* location,
size_t location_len,
const char* message,
size_t message_len) {
log_error("Fatal error in Node-API: %.*s: %.*s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps put the message first and the location in a parenthesis afterwards? Just to get to the meat of the issue first.

Suggested change
log_error("Fatal error in Node-API: %.*s: %.*s",
log_error("Fatal error in Node-API: %.*s (in %.*s)",

static_cast<int>(location_len),
location,
static_cast<int>(message_len),
message);
abort();
}

napi_status napi_get_node_version(
node_api_basic_env env, const napi_node_version** result) {
if (!result) {
return napi_invalid_arg;
}

static napi_node_version version = {
.major = NODE_MAJOR_VERSION,
.minor = NODE_MINOR_VERSION,
.patch = NODE_PATCH_VERSION,
};
*result = &version;
return napi_ok;
}

napi_status napi_get_version(node_api_basic_env env, uint32_t* result) {
if (!result) {
return napi_invalid_arg;
}

*result = NAPI_VERSION;
return napi_ok;
}

} // namespace callstack::nodeapihost
10 changes: 10 additions & 0 deletions packages/host/cpp/RuntimeNodeApi.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,14 @@ napi_status napi_create_external_buffer(napi_env env,
node_api_basic_finalize basic_finalize_cb,
void* finalize_hint,
napi_value* result);

void __attribute__((noreturn)) napi_fatal_error(const char* location,
size_t location_len,
const char* message,
size_t message_len);

napi_status napi_get_node_version(
node_api_basic_env env, const napi_node_version** result);

napi_status napi_get_version(node_api_basic_env env, uint32_t* result);
} // namespace callstack::nodeapihost
7 changes: 7 additions & 0 deletions packages/host/cpp/Versions.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#pragma once

#define NODE_MAJOR_VERSION 8
#define NODE_MINOR_VERSION 6
#define NODE_PATCH_VERSION 0

#define NAPI_VERSION 1
3 changes: 3 additions & 0 deletions packages/host/scripts/generate-weak-node-api-injector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ const IMPLEMENTED_RUNTIME_FUNCTIONS = [
"napi_queue_async_work",
"napi_delete_async_work",
"napi_cancel_async_work",
"napi_fatal_error",
"napi_get_node_version",
"napi_get_version",
];

/**
Expand Down
Loading