Add JsConsoleLogger so native code can log to the JS console - #224
Add JsConsoleLogger so native code can log to the JS console#224bkaradzic-microsoft wants to merge 5 commits into
Conversation
Native diagnostics written to stdout/stderr are invisible on the platforms where they matter most: on Android and iOS there is no attached terminal, so the message reaches nobody. Routing through the JS console instead puts native messages wherever the host has already directed the script's own. BabylonNative has had this utility for a while, but it lives inside the NativeEngine plugin's private Source directory, so nothing else can use it -- the Canvas polyfill currently writes its font and diagnostic warnings to stderr for exactly that reason. Move it here, to Core/JsRuntime, where it only needs napi and any consumer can reach it. Behavior is unchanged apart from LogMethod, which was a namespace-scope function with external linkage declared in no header; it is now in an anonymous namespace.
There was a problem hiding this comment.
Pull request overview
This PR introduces Babylon::JsConsoleLogger, a small utility in Core/JsRuntime that allows native code to route diagnostics through the JavaScript console (helpful on platforms where stdout/stderr are not visible). It also adds unit tests to validate routing behavior and the documented no-op behavior when console (or a method) is missing.
Changes:
- Added
JsConsoleLogger(header + implementation) toCore/JsRuntime. - Added two unit tests covering routing to the Console polyfill callback and the no-console/no-method no-op path.
- Updated CMake wiring so unit tests link against
JsRuntime.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Adds unit tests for JsConsoleLogger behavior. |
| Tests/UnitTests/CMakeLists.txt | Links UnitTests against JsRuntime to access the new logger. |
| Core/JsRuntime/Source/JsConsoleLogger.cpp | Implements JsConsoleLogger by calling console.log/warn/error. |
| Core/JsRuntime/Include/Babylon/JsConsoleLogger.h | Declares the JsConsoleLogger utility API and behavior contract. |
| Core/JsRuntime/CMakeLists.txt | Adds the new header/source to the JsRuntime target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| void LogMethod(Napi::Env env, const char* methodName, const char* message) | ||
| { | ||
| auto console = env.Global().Get("console"); | ||
|
|
||
| if (console.IsObject()) | ||
| { | ||
| auto consoleLog{console.ToObject().Get(methodName)}; | ||
|
|
||
| if (consoleLog.IsFunction()) | ||
| { | ||
| auto consoleLogFunction = consoleLog.As<Napi::Function>(); | ||
| auto messageStr = Napi::String::New(env, message); | ||
| consoleLogFunction.Call(console, {messageStr}); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Agreed, and the precedent you point at is the right one -- fixed in e0ed6a1.
I wrapped the whole lookup-and-call in try/catch (...) and cleared any exception left pending on env. Both are needed, and the test I added shows why. In this build node-addon-api converts the failure into a C++ throw and clears the pending exception as part of that conversion, so IsExceptionPending() alone observes nothing -- the escaping C++ exception is the real failure mode. In a build without C++ exceptions there is no conversion and the exception stays pending instead, which is the case you described.
Worth recording that my first attempt at the test installed the throwing accessor from JS via loader.Eval and passed against the unfixed code, so it was proving nothing. Installing the accessor from C++ inside the same dispatch makes it deterministic, and reverting the guard now fails it with throwing console getter escaped as a C++ exception; throwing console.warn escaped as a C++ exception;, covering both the accessor and the call.
There was a problem hiding this comment.
Follow-up on this thread, since getting the test to run everywhere turned up something worth recording.
Win32_x64_JSI failed three times on the way here, each for a different reason:
Napi::Object::DefinePropertyandNapi::PropertyDescriptordo not exist in the JSI Node-API port, so it would not compile. Went through JS's ownObject.definePropertyinstead.- Throwing out of a
Napi::Functioncallback aborts the process under JSI (Fatal error in v8::ToLocalChecked: Empty MaybeLocal), so the throwers could not be host functions. Built them as real JS functions vianew Function(...). - Even then it aborts -- the shim raises the same fatal error for any JS exception raised through a native property access, so the process dies inside
env.Global().Get("console")beforeLogMethodgets control.
(3) is not something this PR can defend against: there is no return path from the abort. So the test now carries a JSRUNTIMEHOST_NAPI_ENGINE_JSI opt-out matching the two that already exist in this file, and it covers Chakra, V8, JavaScriptCore and QuickJS. The guard in LogMethod stays unconditional.
Worth being explicit that this means a throwing console is fatal under JSI regardless of what any caller does. Happy to file that separately if it is not already known.
Every step of the log path runs script the host does not control: `console` and the method looked up on it can be accessors that throw, and the call itself is arbitrary user code. Any of that escaping means a diagnostic helper corrupts whatever the caller was doing. Swallow the C++ exception and clear any exception left pending on `env`. Both are needed: node-addon-api clears the pending exception when it converts one into a C++ throw, but that conversion does not happen in a build without C++ exceptions, where the exception stays pending instead and would surface at some unrelated later point. This mirrors the guard already on Console::CaptureCurrentJsStack.
Napi::Object::DefineProperty and Napi::PropertyDescriptor do not exist in the JSI Node-API port, so Win32_x64_JSI failed to compile. Go through JS's own Object.defineProperty instead, which every engine has. Reverting the guard still fails the test with both cases reported, so it keeps its A/B value.
Throwing out of a Napi::Function callback aborts the process under the JSI
port ("Fatal error in v8::ToLocalChecked: Empty MaybeLocal"), so building
the throwing accessor and the throwing console.warn out of host functions
crashed Win32_x64_JSI. Construct them as real JS functions instead.
Reverting the guard still fails the test with both cases reported.
The V8JSI Node-API shim aborts the process on any JS exception raised
through a native property access ("Fatal error in v8::ToLocalChecked:
Empty MaybeLocal"), so it dies inside env.Global().Get("console") before
LogMethod can guard anything. No amount of guarding in JsConsoleLogger can
survive that, so exercise this on the other backends only, matching the
existing JSRUNTIMEHOST_NAPI_ENGINE_JSI opt-outs.
The guard itself stays unconditional: it is what protects the Chakra, V8,
JavaScriptCore and QuickJS backends.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Core/JsRuntime/Source/JsConsoleLogger.cpp:9
- Check for a pre-existing pending exception before starting the log operation. As written, the cleanup below clears any pending exception, not only one raised by
console; a caller that invokes this afterThrowAsJavaScriptException()loses its original error, andJsRuntime::Dispatchcan no longer propagate it as intended (Core/JsRuntime/Source/JsRuntime.cpp:52-58). Return without logging when the environment is already exceptional, while retaining the existing cleanup for exceptions created during this call.
try
Core/JsRuntime/Include/Babylon/JsConsoleLogger.h:23
- Apply
BABYLON_APIto these new public entry points and includeBabylon/Api.h. Public cross-library functions consistently declare this calling convention (for example,Core/JsRuntime/Include/Babylon/JsRuntime.h:20,39-40andPolyfills/Console/Include/Babylon/Polyfills/Console.h:29,49); omitting it can give Windows consumers compiled with a non-cdecl default an ABI mismatch.
static void LogInfo(Napi::Env env, const char* message);
static void LogWarn(Napi::Env env, const char* message);
static void LogError(Napi::Env env, const char* message);
Groundwork for a BabylonNative follow-up to BabylonJS/BabylonNative#1824, where this was agreed in review.
Why
Native diagnostics written to
stdout/stderrare invisible on the platforms where they matter most: on Android and iOS there is no attached terminal, so the message reaches nobody. Routing them through the JS console instead puts native messages wherever the host has already directed the script's own.BabylonNative has had exactly this utility for a while, but it lives in
Plugins/NativeEngine/Source/, which is private to that plugin, so nothing else can use it. The Canvas polyfill writes its font and diagnostic warnings tostderrfor that reason alone.What
Moved it to
Core/JsRuntime, where it needs onlynapiand any consumer can reach it. The BabylonNative side (bumping the pin, deleting its private copy, and routing the Canvas warnings through it) follows separately.Behavior is unchanged, with one exception:
LogMethodwas a namespace-scope function with external linkage that was declared in no header, so it was an unnecessary exported symbol and a potential ODR collision. It is now in an anonymous namespace.Tests
Added two, since the type had none:
JsConsoleLogger.RoutesToConsole— asserts all three methods reach the Console polyfill callback with the rightLogLeveland message, in order.JsConsoleLogger.NoConsoleIsNotFatal— the documented no-op path, covering both noconsoleat all and aconsolewhosewarnis not a function.Full suite locally (Win32/Chakra): 12/12 gtest suites (was 10), 216 JS assertions passing.