feat: budgeted console formatter (inspect builtin) - #1991
Open
edusperoni wants to merge 1 commit into
Open
Conversation
console.* now formats objects through internal/inspect.js, a util.inspect-lite compiled once per isolate: depth 2 (console.dir 4), 100 entries per collection, 10k characters per string and a 16KB hard cap, so no single log call can hang the app on a large or cyclic graph. Cycle detection tracks in-progress ancestors only, so shared acyclic references still print in full and only true cycles say [Circular]. Getters are never invoked - accessors render as [Getter]/[Setter] - with two guarded exceptions: error.stack, and a custom toString override, which keeps NativeScript core's Button(42) convention working. All intrinsic access goes through primordials and brands come from a captured Object.prototype.toString, so a tampered prototype cannot change or break the output. binding.getNativeWrapperHint is the Android-specific piece: objects backed by a Java counterpart render as a short [java.lang.Object] hint instead of having their native-backed graph walked, and package objects render as [package java.lang] rather than materializing every class they contain. Detection reads private symbols and internal fields only, so it never runs JS and cannot throw. This replaces the JSON-based path: smart-stringify.js, JsonStringifyObject and transformJSObject are gone, along with console's hand-rolled array printer and console.dir's duplicate dump logic. Logs now look like Node inspect output instead of pretty-printed JSON. If the formatter fails to initialize, logging degrades to ToDetailString rather than throwing. Mirrors NativeScript/ios#416.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Android mirror of NativeScript/ios#416. Stacked on #1990 (
feat/primordials) — review only the last commit.Replaces the console's logcat formatting pipeline — which could hang the app on a single
console.log— withinternal/inspect.js, a budgeted util.inspect-lite built on primordials. The DevTools path (inspector formats raw values itself) is untouched.The problem
transformJSObjectfell back toJSON.stringify(obj, replacer, 2)with no depth or size limit — logging a store/component tree serialized everything reachable.seenarray made cycle tracking O(n²) and never popped on subtree exit, so shared acyclic references falsely printed[Circular].JSON.stringifyinvokes getters andtoJSON— a log line could execute arbitrary user code.console.dirhad its own duplicate dump with the same issues.The fix (
inspect.js, ported verbatim from iOS — function-body builtin, primordials-clean):diruses 4), 100 entries per collection, 10k chars per string, 16KB hard cap with an explicit truncation marker.Set(enter/exit) — true cycles say[Circular], diamonds print normally.[Getter]/[Setter]tags), with two guarded exceptions:error.stack, and customtoStringoverrides — NativeScript core'sViewBase/Observableconvention (Button(42)) keeps working, detected via descriptors and invoked guarded + capped.Object.prototype.toString, Map/Set walked through captured iteratornext, sizes via captured accessor getters.__inspectglobal.binding.getNativeWrapperHint— the Android-specific piece. Java-backed objects render as short hints instead of having their native-backed graphs walked:[java.lang.Object],[java.util.ArrayList],[package java.lang]; Java classes print[Function: java.lang.Object]via their existingSetClassName. Detection reads private symbols and internal fields only — no JNI, no JS execution, cannot throw. Two hazards handled along the way:InternalFieldCount() == 2alone false-positives on typed arrays/ArrayBuffers (V8 gives them the same embedder-field count); the hint additionally requires the field to hold a realExternal.getOwnPropertyDescriptorwould invoke —console.log(java)would have materialized every class in every package. Packages now carry a private marker and short-circuit to[package …].Deleted:
smart-stringify.js,GetSmartJSONStringifyFunction,JsonStringifyObject(console was its only caller),transformJSObject, the array printer, anddir's duplicate dump. If the formatter ever fails to initialize, logging degrades toToDetailString.Output changes to be aware of: logs now look like Node's inspect output (
{ a: 1, self: [Circular] },Map(2) { … }, truncation markers) instead of pretty-printed JSON;console.dirkeeps its dump markers but prints one budgeted rendering at depth 4; Java arrays render as a hint instead of being walked.Related Pull Requests
Does your pull request have unit tests?
Yes — 14 new device specs (
tests/testInspect.js): cycles vs diamonds, caps/truncation, getter non-invocation, native-wrapper hints on real Java objects, tampered-prototype formatting, and a bounded-time regression (console.logof a ~5000-node cyclic graph). Full suite: 627 specs, 0 failures (613 baseline + 14).