feat: ns:util builtin module (inspect, format) - #418
Conversation
📝 WalkthroughWalkthroughThe runtime adds registered ChangesBuiltin module runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BuiltinRequire
participant RequireCallback
participant NsBuiltinModules
participant SyntheticModule
BuiltinRequire->>RequireCallback: resolve builtin specifier
RequireCallback->>NsBuiltinModules: GetExports(specifier)
NsBuiltinModules->>SyntheticModule: create or retrieve builtin module
SyntheticModule-->>NsBuiltinModules: module exports
NsBuiltinModules-->>RequireCallback: cached exports
RequireCallback-->>BuiltinRequire: return exports or throw not-found error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
732ad4e to
1da68c5
Compare
1da68c5 to
8539121
Compare
8badc20 to
e39fea4
Compare
Runtime-provided modules now resolve under the URL-style `ns:` scheme,
ahead of any filesystem or npm resolution, with `node:` compatibility
shims served from the same registry. v1 ships `ns:util` (inspect, format)
and a `node:util` shim re-exporting its members from a distinct, frozen
module object.
Each specifier is one source file and one registry entry: a shim is its
own builtin that reaches the `ns:` module it adapts through a new fixed
wrapper parameter, `require`, which resolves builtin specifiers and
nothing else. Shims therefore compile only when first resolved and own
every bit of Node adaptation, keeping compatibility knowledge out of the
standard modules.
format() is Node's util.format (%s %d %i %f %j %o %O %%, extras appended
space-separated), and console.* routes its arguments through it, so
`console.log("%d apples", 3)` works while unknown and dangling percent
signs stay verbatim.
Resolution is wired into the CommonJS require path, the ES module resolve
callback and the dynamic-import callback; ESM consumption is served by a
per-realm synthetic module. An unknown name in either scheme fails with
`No such built-in module: <specifier>`, which replaces the warn-and-export-
nothing polyfill previously handed to unshimmed `node:` imports. Bare
specifiers are untouched.
docs/ns-builtin-modules.md is the cross-runtime contract both runtimes
implement.
…s inputs Builtins stay classic function bodies on both runtimes; cross-builtin sharing, if ever needed, starts with generation-time bundling. The js2c guard turns an accidental .mjs into a build error pointing at the spec.
e39fea4 to
dd7115d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/runtime/Caches.h (1)
158-186: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the new Persistent caches in
~Caches.
Caches::~Caches()currently only clears the preexisting prototype/cache maps and bound objects. The new entries inBuiltinModuleExportsandBuiltinModulesalso holdv8::Persistents, and the single handlesFormatFuncandBuiltinRequireneed the same teardown. Add per-valueReset()cleanup for the maps and reset these single handles before isolate disposal.robin_hood::unordered_setis provided by the bundledrobin_hood.h, so that part needs no change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Caches.h` around lines 158 - 186, Update Caches::~Caches() to reset every Persistent introduced by the new caches before isolate disposal: iterate BuiltinModuleExports and BuiltinModules and call Reset() on each stored handle, then reset the single handles FormatFunc and BuiltinRequire. Preserve the existing cleanup for other caches and leave BuiltinModulesInProgress unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ns-builtin-modules.md`:
- Around line 27-29: Update the builtin-module resolution documentation to
distinguish the APIs: state that require() throws synchronously for an unknown
builtin, while dynamic import() rejects its promise with the same Error message,
`No such built-in module: ns:<name>`. Preserve the exact message wording.
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 218-236: Update the builtin-resolution predicate in
RequireCallback to match ESM behavior: use the new builtin path for registered
modules and ns:-prefixed specifiers, while allowing unregistered node:-prefixed
specifiers to continue through the legacy node:url fallback. Reuse the existing
ESM branch predicate or document and centralize the shared condition so CommonJS
and ESM resolve node: specifiers consistently.
In `@NativeScript/runtime/NsBuiltinModules.cpp`:
- Around line 272-301: Update the failure path in
NsBuiltinModules::GetFormatFunc so FormatFuncUnavailable is set only when
BuiltinModulesInProgress does not contain "ns:util"; preserve the existing error
logging and empty-function return, but avoid permanently caching failures caused
by re-entrant ns:util initialization.
In `@TestRunner/app/tests/NsUtilTests.js`:
- Around line 86-91: Update the “backs console.* formatting” test to spy on or
capture the supported console output sink instead of only asserting no throw.
Verify the first console.log call emits “3 apples” and the second emits “100%”
unchanged, using the test runner’s existing console-spy mechanism if available.
---
Outside diff comments:
In `@NativeScript/runtime/Caches.h`:
- Around line 158-186: Update Caches::~Caches() to reset every Persistent
introduced by the new caches before isolate disposal: iterate
BuiltinModuleExports and BuiltinModules and call Reset() on each stored handle,
then reset the single handles FormatFunc and BuiltinRequire. Preserve the
existing cleanup for other caches and leave BuiltinModulesInProgress unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af47a8ca-636b-4ffb-8730-f207028b0a7c
📒 Files selected for processing (21)
NativeScript/runtime/BuiltinLoader.cppNativeScript/runtime/BuiltinLoader.hNativeScript/runtime/Caches.hNativeScript/runtime/Console.cppNativeScript/runtime/Console.hNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/ModuleInternalCallbacks.mmNativeScript/runtime/NsBuiltinModules.cppNativeScript/runtime/NsBuiltinModules.hNativeScript/runtime/js/README.mdNativeScript/runtime/js/node-util.jsNativeScript/runtime/js/ns-util.jsNativeScript/runtime/js/primordials.jsTestRunner/app/tests/NsUtilTests.jsTestRunner/app/tests/esm/ns-util-import.mjsTestRunner/app/tests/index.jsdocs/ns-builtin-modules.mdeslint.config.mjstools/js2c-inputs.xcfilelisttools/js2c.mjsv8ios.xcodeproj/project.pbxproj
| - Resolution of an unknown builtin fails synchronously with an `Error` whose | ||
| message is exactly `No such built-in module: ns:<name>` (matching Node's | ||
| wording for familiarity). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish CommonJS errors from dynamic-import rejection.
The synchronous-error statement is not correct for import("ns:definitely-not-a-module"). Dynamic import returns a promise and reports this error by rejection, as TestRunner/app/tests/NsUtilTests.js lines 109-118 verify. State that require() throws synchronously and dynamic import() rejects with the same Error message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/ns-builtin-modules.md` around lines 27 - 29, Update the builtin-module
resolution documentation to distinguish the APIs: state that require() throws
synchronously for an unknown builtin, while dynamic import() rejects its promise
with the same Error message, `No such built-in module: ns:<name>`. Preserve the
exact message wording.
| // Builtin modules resolve before any path handling, so they can never be | ||
| // shadowed by a file or a package, and an unknown one fails as a missing | ||
| // builtin rather than as a missing file. Only prefixed specifiers get here: | ||
| // a bare `util` still resolves through npm. | ||
| if (info.Length() > 0 && info[0]->IsString()) { | ||
| std::string specifier = tns::ToString(isolate, info[0].As<v8::String>()); | ||
| if (NsBuiltinModules::IsBuiltinScheme(specifier)) { | ||
| Local<Context> context = isolate->GetCurrentContext(); | ||
| Local<Object> exports; | ||
| if (NsBuiltinModules::GetExports(context, specifier).ToLocal(&exports)) { | ||
| info.GetReturnValue().Set(exports); | ||
| } else if (!NsBuiltinModules::IsRegistered(specifier)) { | ||
| isolate->ThrowException(Exception::Error( | ||
| tns::ToV8String(isolate, NsBuiltinModules::NotFoundMessage(specifier)))); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect IsBuiltinScheme / IsNsScheme / IsRegistered definitions and the node:url legacy polyfill.
rg -n -B2 -A15 'bool.*IsBuiltinScheme|bool.*IsNsScheme|bool.*IsRegistered' NativeScript/runtime/NsBuiltinModules.h NativeScript/runtime/NsBuiltinModules.cpp 2>/dev/null
rg -n 'node:url|node:util' NativeScript/runtime/NsBuiltinModules.cpp docs/ns-builtin-modules.md 2>/dev/nullRepository: NativeScript/ios
Length of output: 4970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ModuleInternal RequireCallback context =="
sed -n '190,250p' NativeScript/runtime/ModuleInternal.mm 2>/dev/null || true
echo
echo "== ModuleInternalCallbacks relevant resolution callbacks =="
rg -n -B8 -A22 'ResolveModuleCallback|ImportModuleDynamicallyCallback|legacy|node:url|node:util|RequireCallback' NativeScript/runtime/ModuleInternalCallbacks.mm 2>/dev/null || true
echo
echo "== NsBuiltinModules constants and prefix check =="
rg -n -B4 -A6 'kNodePrefix|kNsPrefix|HasPrefix' NativeScript/runtime/NsBuiltinModules.cpp 2>/dev/null || true
echo
echo "== Deterministic predicate/probe from source text =="
python3 - <<'PY'
from pathlib import Path
import re
ns_cpp = Path('NativeScript/runtime/NsBuiltinModules.cpp').read_text(errors='replace')
m = re.search(r'bool NsBuiltinModules::IsBuiltinScheme\(const std::string& specifier\) \{\s*(.*?)\n\}', ns_cpp, re.S)
print('IsBuiltinScheme body:', m.group(0).strip() if m else 'NOT_FOUND')
m = re.search(r'bool NsBuiltinModules::IsNsScheme\(const std::string& specifier\) \{\s*(.*?)\n\}', ns_cpp, re.S)
print('IsNsScheme body:', m.group(0).strip() if m else 'NOT_FOUND')
m = re.search(r'bool.+kNodePrefix.+=(.+?);|kNodePrefix\s*=\s*([^;]+);', \
'\n'.join(re.findall(r'k[a-zA-Z0-9_]+Prefix\s*=[^;]+;', ns_cpp)), re.S)
prefixes = re.findall(r'k[Nn]ode[Nn]Prefix(?:\s*=\s*\"([^\"]+)\"|:\s*\"([^\"]+)")', ns_cpp)
print('prefixes text:', prefixes)
def has_prefix(specifier, prefix):
return specifier.startswith(prefix)
def predicate_new(specifier):
# Derived from current IsBuiltinScheme body
return has_prefix(specifier, 'ns:') or has_prefix(specifier, 'node:')
def predicate_esm_condition(specifier, registered):
# Derived from IsRegistered(rawSpec) || IsNsScheme(rawSpec) branch
return registered or has_prefix(specifier, 'ns:')
for specifier in ['node:url', 'node:util', 'ns:url', 'util', 'app/builtin']:
registered = specifier in {'node:url', 'node:util'} # registry lookup not provided; use documented presence
print(specifier, 'new_predicate=', predicate_new(specifier),
'esm_condition=', predicate_esm_condition(specifier, registered),
'registered=', registered)
PYRepository: NativeScript/ios
Length of output: 33628
Align RequireCallback builtin-prefixed resolution with ESM paths.
RequireCallback now treats every ns: or node: specifier as builtin and rejects unregistered keys as missing modules. ESM resolution only takes the new builtin path for registered nodes and ns:-prefixed specifiers, leaving unregistered node: specifiers to the legacy node:url fallback. Use or document the same branch predicate for both paths so CommonJS and ESM handle the same node: specifiers consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/ModuleInternal.mm` around lines 218 - 236, Update the
builtin-resolution predicate in RequireCallback to match ESM behavior: use the
new builtin path for registered modules and ns:-prefixed specifiers, while
allowing unregistered node:-prefixed specifiers to continue through the legacy
node:url fallback. Reuse the existing ESM branch predicate or document and
centralize the shared condition so CommonJS and ESM resolve node: specifiers
consistently.
| Local<v8::Function> NsBuiltinModules::GetFormatFunc(Local<Context> context) { | ||
| Isolate* isolate = v8::Isolate::GetCurrent(); | ||
| std::shared_ptr<Caches> cache = Caches::Get(isolate); | ||
| if (cache->FormatFunc != nullptr) { | ||
| return cache->FormatFunc->Get(isolate); | ||
| } | ||
| if (cache->FormatFuncUnavailable) { | ||
| return Local<v8::Function>(); | ||
| } | ||
|
|
||
| TryCatch tc(isolate); | ||
| Local<Object> exports; | ||
| Local<Value> format; | ||
| if (!GetExports(context, "ns:util").ToLocal(&exports) || | ||
| !exports->Get(context, tns::ToV8String(isolate, "format")) | ||
| .ToLocal(&format) || | ||
| !format->IsFunction()) { | ||
| if (tc.HasCaught()) { | ||
| tns::LogError(isolate, tc); | ||
| } | ||
| // One attempt per realm: a broken builtin must not make every log call | ||
| // recompile it. | ||
| cache->FormatFuncUnavailable = true; | ||
| return Local<v8::Function>(); | ||
| } | ||
|
|
||
| cache->FormatFunc = std::make_unique<Persistent<v8::Function>>( | ||
| isolate, format.As<v8::Function>()); | ||
| return format.As<v8::Function>(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'Console.cpp' --exec rg -n -C8 'InitInspect|GetFormatFunc|FormatFunc|Log|NSLog' {}Repository: NativeScript/ios
Length of output: 11507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant source files without executing repository code.
fd -t f '(NsBuiltinModules|Caches|BuiltinLoader|NativeScriptRuntime|RuntimeConfig)\.(cpp|h|hpp)$' .
printf '\n--- NsBuiltinModules outlines ---\n'
for f in $(fd -t f 'NsBuiltinModules\.(cpp|h|hpp)$' .); do
echo "FILE $f"
ast-grep outline "$f" --view compact || true
done
printf '\n--- Relevant NsBuiltinModules lines ---\n'
fd -t f 'NsBuiltinModules\.cpp$' . --exec sh -c 'echo "FILE $1"; sed -n "1,360p" "$1" | cat -n' sh {}
printf '\n--- Relevant Console.cpp section ---\n'
fd -t f 'Console\.cpp$' . --exec sh -c 'echo "FILE $1"; sed -n "60,290p" "$1" | cat -n' sh {}
printf '\n--- Searches for BuiltinModulesInProgress / InProgress / GetExports ---\n'
rg -n -C3 'BuiltinModulesInProgress|In.*(Progress|Loading)|Is.*Loading|LoadBuiltin|GetExports|FormatFuncUnavailable|FormatFunc' .Repository: NativeScript/ios
Length of output: 50373
Do not treat re-entrant GetExports failures as permanent.
GetFormatFunc() can fail while ns:util is being instantiated because BuildBinding() calls Console::InitInspect() before returning exports. If this re-entry throws through GetExports("ns:util"), GetFormatFunc() sets FormatFuncUnavailable = true for the realm even though ns:util later completes successfully. Skip latching the flag when BuiltinModulesInProgress.count("ns:util") is non-zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/NsBuiltinModules.cpp` around lines 272 - 301, Update the
failure path in NsBuiltinModules::GetFormatFunc so FormatFuncUnavailable is set
only when BuiltinModulesInProgress does not contain "ns:util"; preserve the
existing error logging and empty-function return, but avoid permanently caching
failures caused by re-entrant ns:util initialization.
| it("backs console.* formatting", function () { | ||
| expect(function () { | ||
| console.log("%d apples", 3); | ||
| console.log("100%"); | ||
| }).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the formatted console output.
This test passes if console.log does not use util.format. Capture the console sink or install the test runner’s supported console spy. Assert that the first call emits 3 apples rather than %d apples, and that 100% remains unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TestRunner/app/tests/NsUtilTests.js` around lines 86 - 91, Update the “backs
console.* formatting” test to spy on or capture the supported console output
sink instead of only asserting no throw. Verify the first console.log call emits
“3 apples” and the second emits “100%” unchanged, using the test runner’s
existing console-spy mechanism if available.
Stacked on #416 (inspect). Review only the last commit; the base branch is
feat/inspect.What
Runtime-provided modules get a resolution scheme.
require("ns:util"),import util, { inspect } from "ns:util"andawait import("ns:util")all resolve to the same per-realm module, andconsole.*gains Node's%-substitution.v1 exports:
inspect(value[, options])format(fmt, ...args)util.format:%s %d %i %f %j %o %O %%, extras appended space-separateddocs/ns-builtin-modules.md(added here) is the cross-runtime contract — the same document the Android runtime implements against, so a capability behaves identically on both platforms before it ships in a stable release.Resolution rules
ns:/node:specifiers are intercepted before any filesystem or npm resolution, in all three entry points: the CommonJSrequirepath (ModuleInternal.mm:219), the ES module resolve callback (ModuleInternalCallbacks.mm:719) and the dynamic-import host callback (ModuleInternalCallbacks.mm:1878). A builtin can never be shadowed by a file or a package.v8::Module::CreateSyntheticModule, exporting every member by name plusdefault(the exports object). The module is instantiated, evaluated and cached per realm, so repeated imports return the identical namespace.No such built-in module: <specifier>— synchronously forrequire, as a rejection forimport().Caches, not in a process-global).node:shimsThe same registry serves
node:(spec: "node:compatibility shims"), so npm packages requiring Node builtins by their prefixed name can run where a shim exists.node:utilis its own builtin (node-util.js) that consumesns:utilthrough the internal require and re-exports its members. The shim owns all the Node adaptation — argument shapes, option names, aliases — sons-util.jscontains no compatibility code and does not know a shim exists. This is the reference pattern every futurens:*module and shim copies.node-util.jsis only compiled the first timenode:utilis resolved, so an app that never touches thenode:scheme never pays for it.node:utilis a distinct, separately frozen module object fromns:util, even though every member is re-exported unchanged (nodeUtil.inspect === nsUtil.inspect) — per the spec bullet, mirroring how Bun (bun:*), Deno and Cloudflare (cloudflare:*) keep their own surface apart from their Node compat layer.ns:utilcan grow runtime-specific exports without widening the Node compatibility surface.typeof util.promisify === "undefined"), never present-but-throwing, so feature checks work.require("util")still resolves through npm exactly as before (many apps bundle theutilpolyfill package). Only the explicit prefix reaches the registry. There is a test assertingrequire("util")does not return the builtin.node:ES imports previously got a generic in-memory polyfill (console.warn(...); export default {}) that broke at first use; they now fail withNo such built-in module: node:<name>. The pre-existingnode:urlpolyfill (fileURLToPath/pathToFileURL) is kept as-is and is still import-only; it is noted in the doc's non-normative iOS section.The internal
require(new wrapper parameter)The builtin function wrapper becomes
(exports, require, module, binding, primordials)— Node's order.requireis created once per realm byBuiltinLoaderand resolves builtin specifiers only: no path, no package, no filesystem. An unregistered name throws exactlyNo such built-in module: <specifier>, and requiring a module that is still loading throws instead of recursing (per-realm in-progress guard). It is documented as normative in the spec, since Android needs it to build shims the same way. C++ call sites are unchanged; builtins that don't need it simply ignore the parameter.console integration
Console::BuildStringFromArgscalls the cachedformatwith the full argument list, soconsole.log("%d apples", 3)prints3 appleswhileconsole.log("100%"), unknown specifiers and a dangling%stay verbatim. Non-format calls keep joining with spaces exactly as before. If the builtin is unavailable the old per-argument loop still runs, so logging can never be taken down by the formatter.Implementation
NativeScript/runtime/js/ns-util.js— thens:utilbuiltin, all intrinsic access through primordials (Number,NumberParseInt,NumberParseFloat,StringPrototypeCharCodeAtadded, ESLint lists kept in sync).inspectis passed in viabinding, so the module and console share one formatter instance.NativeScript/runtime/js/node-util.js— the shim, four lines of re-export plus the rule about where adaptation code lives.NativeScript/runtime/NsBuiltinModules.{h,cpp}— the registry (one specifier → oneBuiltinId), lazy per-realm materialization with a cycle guard, and the synthetic-module evaluation steps. Adding a module is one row plus one file.Stability caveat
Verbatim from Node's contract, and repeated in the doc:
inspect's output (and thereforeformat's object rendering) may change between runtime versions for readability. It is for humans and must not be parsed programmatically.Tests
TestRunner/app/tests/NsUtilTests.js(+ an.mjsfixture that statically imports both specifiers, so the resolve callback is covered and not just the dynamic-import fast path): frozen exports, singleton identity, every format specifier,%%/unknown/dangling%,%jon a circular value, extras, non-string first argument,console.log("%d apples", 3), dynamic + static import identity,node:utildistinctness with shared members, unknownns:/node:messages, and the bare-specifier non-change.Full suite on the simulator: 941 tests, 0 failures (922 baseline + 19 new).
formatwas additionally verified against Node's ownutil.formaton the host for every non-object case.Summary by CodeRabbit
New Features
ns:utilandnode:utilmodules with inspection and formatting utilities.requirefor registered built-in modules.Documentation
Tests