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.
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);
|
Some code like 3rd party libraries will log to stdout/stderr and we won't be able to see that. So instead of having our own log function, I think it's better to route stdout/err to platform specific output like this : https://github.com/BabylonJS/AndroidExtensions/blob/main/Source/StdoutLogger.cpp |
|
Agreed. Redirecting stdout/stderr at the platform host is broader and also catches third-party native output; this helper only helps call sites we explicitly migrate. JsRuntimeHost already uses |
|
Reworked this around Cedric's stream-forwarding suggestion and replaced the old This is not Android/Apple-only: Windows preserves the existing destination and mirrors to The router now lives in GitHub would not allow this PR to be reopened after its branch was replaced, so the cross-platform implementation is now in #233. |
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.