| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughRuntime JavaScript is generated into C++ builtin sources and loaded through a cached BuiltinLoader. Runtime initialization paths now use these builtins, while enum and global metadata evaluations gain memoization. ChangesRuntime builtins
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Xcode
participant js2c
participant BuiltinLoader
participant V8
Xcode->>js2c: Generate RuntimeBuiltins
js2c->>Xcode: Provide generated C++ sources
Xcode->>BuiltinLoader: Build builtin loader
BuiltinLoader->>V8: Compile or consume cached builtin
V8-->>BuiltinLoader: Execute builtin and return module.exports
Possibly related PRs
Suggested reviewers: nathanwalker Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)tools/js2c-inputs.xcfilelist (1)🤖 Prompt for all review comments with AI agentsNativeScript/runtime/js/require-factory.js (1)1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
New builtin .js files must be added here too, or incremental builds silently skip regeneration.
This file drives Xcode's incremental-build change detection for the "Generate RuntimeBuiltins" phase, but the phase's shell script actually globs NativeScript/runtime/js/*.js at runtime. If a contributor adds a new builtin script without updating this list, Xcode won't detect it as a changed input and may skip re-running js2c.mjs, leaving the new file out of the generated RuntimeBuiltins.{h,cpp} until an unrelated input changes or a clean build happens.
Consider adding a short comment here (or in tools/js2c.mjs's usage text) noting that this list must be kept in sync with NativeScript/runtime/js/*.js.
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/js2c-inputs.xcfilelist` around lines 1 - 12, Add a concise maintenance comment to the input list documenting that every new NativeScript/runtime/js/*.js builtin must also be added here so Xcode incremental builds rerun js2c.mjs. Keep the existing file entries and generation behavior unchanged.NativeScript/runtime/js/smart-stringify.js (1)1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Optional: tidy the formatting carried over from the C++ string literal.
The stray line break inside the if body (Lines 4-5) and trailing whitespace are artifacts of the old embedded literal. Now that this is a real source file, normalizing it costs nothing and keeps future diffs readable. Semantics unchanged.
♻️ Proposed cleanup🤖 Prompt for AI Agents-(function() { - function require_factory(requireInternal, dirName) { - return function require(modulePath) { - if(global.__pauseOnNextRequire) { debugger; -global.__pauseOnNextRequire = false; } - return requireInternal(modulePath, dirName); - } - } - return require_factory; -})() +(function () { + function require_factory(requireInternal, dirName) { + return function require(modulePath) { + if (global.__pauseOnNextRequire) { + debugger; + global.__pauseOnNextRequire = false; + } + return requireInternal(modulePath, dirName); + }; + } + return require_factory; +})();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/js/require-factory.js` around lines 1 - 10, Normalize formatting in the require_factory wrapper, especially the __pauseOnNextRequire conditional, by removing the embedded line break and trailing whitespace while preserving debugger invocation, flag reset, and requireInternal behavior.NativeScript/runtime/Helpers.mm (1)3-13: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Optional: seen as an array yields O(n²) scans and false [Circular] hits for shared (non-cyclic) references.
seen.indexOf(value) is linear per visited object, and entries are never popped when leaving a subtree, so a repeated-but-acyclic reference is reported as circular. Swapping to a Set fixes the complexity; fixing the false positives needs an ancestor stack instead. Both change existing output, so this is fine to defer given the PR's extraction-fidelity goal.
🤖 Prompt for AI AgentsVerify 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/js/smart-stringify.js` around lines 3 - 13, Defer changes to the circular-reference tracking in the replacer function: retain the existing seen array and output behavior for this extraction-focused change. Do not alter complexity or shared-reference handling unless a follow-up explicitly scopes those behavioral changes.NativeScript/runtime/WeakRef.cpp (1)497-503: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider a TryCatch around the builtin run.
If RunBuiltin fails (compile/run error), the scheduled exception stays pending on the isolate while this function returns an empty handle. JsonStringifyObject then continues into JSON::Stringify, so the failure surfaces at an unrelated site. Wrapping this call in a local TryCatch (and logging via tns::LogError) keeps the failure contained, matching the pattern used elsewhere in this file.
♻️ Proposed refactor🤖 Prompt for AI AgentsLocal<Value> result; - bool success = BuiltinLoader::RunBuiltin(context, BuiltinId::kSmartStringify).ToLocal(&result); - tns::Assert(success, isolate); - - if (result.IsEmpty() || !result->IsFunction()) { + TryCatch tc(isolate); + bool success = BuiltinLoader::RunBuiltin(context, BuiltinId::kSmartStringify).ToLocal(&result); + if (!success || tc.HasCaught()) { + tns::LogError(isolate, tc); + return Local<v8::Function>(); + } + + if (result.IsEmpty() || !result->IsFunction()) { return Local<v8::Function>(); }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/Helpers.mm` around lines 497 - 503, In the builtin-loading flow around BuiltinLoader::RunBuiltin, add a local v8::TryCatch and handle compile/run failures before returning an empty function handle. Log the caught exception through tns::LogError, clear or contain the pending isolate exception, and preserve the existing result.IsEmpty()/IsFunction() validation for successful execution.10-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider capturing exception details before asserting, like TSHelpers::Init.
Unlike TSHelpers::Init (which wraps the call in a TryCatch and calls tns::LogError(isolate, tc) before asserting), this path asserts on bare success with no way to see why the WeakRef builtin failed to compile/run. Since WeakRef is treated as similarly critical (hard Assert), aligning the diagnostics would make a future failure easier to debug.
♻️ Proposed alignment with TSHelpers::Init🤖 Prompt for AI Agentsvoid WeakRef::Init(Local<Context> context) { Isolate* isolate = v8::Isolate::GetCurrent(); + TryCatch tc(isolate); Local<Value> result; - bool success = - BuiltinLoader::RunBuiltin(context, BuiltinId::kWeakRef).ToLocal(&result); - tns::Assert(success, isolate); + if (!BuiltinLoader::RunBuiltin(context, BuiltinId::kWeakRef).ToLocal(&result) && + tc.HasCaught()) { + tns::LogError(isolate, tc); + } + tns::Assert(!result.IsEmpty(), isolate); }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/WeakRef.cpp` around lines 10 - 19, Update WeakRef::Init to wrap BuiltinLoader::RunBuiltin in a v8::TryCatch, log the captured exception with tns::LogError(isolate, tc) when execution fails, then retain the existing tns::Assert(success, isolate) behavior.
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 `@NativeScript/runtime/Interop.mm`: - Around line 630-635: Update the enum evaluation flow around script->Run(context) so a failed ToLocal conversion is asserted or handled before result is dereferenced. Remove the result.IsEmpty() condition that suppresses failure handling, and ensure result->IsNumber() is reached only when result contains a valid value. --- Nitpick comments: In `@NativeScript/runtime/Helpers.mm`: - Around line 497-503: In the builtin-loading flow around BuiltinLoader::RunBuiltin, add a local v8::TryCatch and handle compile/run failures before returning an empty function handle. Log the caught exception through tns::LogError, clear or contain the pending isolate exception, and preserve the existing result.IsEmpty()/IsFunction() validation for successful execution. In `@NativeScript/runtime/js/require-factory.js`: - Around line 1-10: Normalize formatting in the require_factory wrapper, especially the __pauseOnNextRequire conditional, by removing the embedded line break and trailing whitespace while preserving debugger invocation, flag reset, and requireInternal behavior. In `@NativeScript/runtime/js/smart-stringify.js`: - Around line 3-13: Defer changes to the circular-reference tracking in the replacer function: retain the existing seen array and output behavior for this extraction-focused change. Do not alter complexity or shared-reference handling unless a follow-up explicitly scopes those behavioral changes. In `@NativeScript/runtime/WeakRef.cpp`: - Around line 10-19: Update WeakRef::Init to wrap BuiltinLoader::RunBuiltin in a v8::TryCatch, log the captured exception with tns::LogError(isolate, tc) when execution fails, then retain the existing tns::Assert(success, isolate) behavior. In `@tools/js2c-inputs.xcfilelist`: - Around line 1-12: Add a concise maintenance comment to the input list documenting that every new NativeScript/runtime/js/*.js builtin must also be added here so Xcode incremental builds rerun js2c.mjs. Keep the existing file entries and generation behavior unchanged.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ac2389-bd4d-4691-9d9f-00fe74a3aa30
📥 CommitsReviewing files that changed from the base of the PR and between 2f47ad9 and 885abe9.
📒 Files selected for processing (32)
Sorry, something went wrong.
|
Review feedback disposition (a56b0d4):
Suite after fixes: 905/905 on the V8 14 base. |
Sorry, something went wrong.
|
✅ Action performed
Review finished.
|
Sorry, something went wrong.
|
Contract change pushed: builtins now export through module.exports instead of a top-level return. The files are compiled as function bodies, so top-level return was legal and worked — but it makes every builtin invalid JavaScript to anything that does not read this repo's ESLint config: editors' TS server, prettier, and CodeRabbit (which flagged it on this PR). No gain to keep it. BuiltinLoader now seeds Node's module wrapper — the fixed parameters are exports, module, binding — ignores the call's return value and hands module.exports back to the C++ call site. Both CommonJS styles work; RunBuiltin's C++ signature and all call sites are unchanged. ESLint moves to sourceType: "script" (no global-return needed) with exports/module declared, and NativeScript/runtime/js/README.md documents the new contract. The three stacked PRs (#415, #416, #418) were rebased onto it. Full suite green on each: 905 / 912 / 922 / 941, 0 failures. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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 `@NativeScript/runtime/js/README.md`: - Around line 41-43: Update the README text describing ESLint’s no-undef rule to say it catches misspelled or undeclared identifiers, while preserving the surrounding guidance about declaring newly used native globals.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47e981e8-2117-4ae4-9ce9-f78ee110c791
📥 CommitsReviewing files that changed from the base of the PR and between a56b0d4 and 4b29896.
📒 Files selected for processing (10)
Sorry, something went wrong.
| - ESLint (`eslint.config.mjs` at the repo root, run by lint-staged) declares | ||
| `exports`, `module`, `binding` and the reachable native globals; `no-undef` | ||
| is the typo net. If a builtin starts using a new native global, add it there. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the no-undef description.
“no-undef is the typo net” is awkward; use wording such as “catches misspelled or undeclared identifiers” for clarity.
🧰 Tools 🪛 LanguageTool[grammar] ~43-~43: Ensure spelling is correct
Context: ...als; no-undef is the typo net. If a builtin starts using a new native global, add i...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI AgentsVerify 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/js/README.md` around lines 41 - 43, Update the README text describing ESLint’s no-undef rule to say it catches misspelled or undeclared identifiers, while preserving the surrounding guidance about declaring newly used native globals.
Source: Linters/SAST tools
Sorry, something went wrong.
…js2c The ~26KB of JavaScript previously embedded as C++ string literals now lives in NativeScript/runtime/js/*.js. A "Generate RuntimeBuiltins" build phase runs tools/js2c.mjs to emit a generated source table, and call sites go through BuiltinLoader::RunBuiltin, which sets proper internal/<name>.js script origins and shares an in-process bytecode cache across isolates so workers stop re-parsing the builtins.
Interop::WriteValue recompiled and re-ran an enum wrapper's __tsEnum snippet on every FFI marshal; the value is now memoized on the EnumDataWrapper. GlobalPropertyGetter did the same on every read of a JsCode global; the evaluated result is now defined as a real own property on the global (the interceptor is kNonMasking, so later reads bypass it entirely), with a reentrancy guard because CreateDataProperty re-enters the interceptor before the property exists.
…rameter
Adopts Node's internals idiom: every builtin is compiled via
ScriptCompiler::CompileFunction with the single parameter `binding`, and
natives arrive as properties of one bag object that each file
destructures at the top (const { isRuntimeRunloop } = binding). The
visible IIFE wrappers are gone, top-level return is the way a builtin
hands a value back to C++, and the bytecode cache moves to
CreateCodeCacheForFunction.
The parameter name is fixed and hardcoded in BuiltinLoader, so there is
no per-builtin name to mistype in C++; on the JS side an ESLint gate
(no-undef with `binding` and the reachable native globals declared)
catches typos in the destructures, wired into lint-staged. Conventions
are documented in NativeScript/runtime/js/README.md.
The enum-evaluation branch in Interop::WriteValue could dereference an empty handle when Run failed (the && !result.IsEmpty() condition suppressed the assert); WeakRef::Init and GetSmartJSONStringifyFunction now log the caught exception before asserting so a builtin failure is diagnosable; js2c gains --filelist, wired into the build phase, so a builtin missing from js2c-inputs.xcfilelist fails the build instead of being silently skipped by incremental change detection.
Builtins are compiled as function bodies, so top-level `return` worked as the export channel -- but it makes every builtin invalid JavaScript to any tool that does not read this repo's ESLint config (editor TS servers, prettier, review bots), for no gain. BuiltinLoader now seeds Node's module wrapper (`exports`, `module`, `binding`), ignores the call's return value and hands `module.exports` back to the C++ call site, so both CommonJS export styles work and the files parse everywhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)NativeScript/runtime/js/ts-helpers.js (1)🤖 Prompt for all review comments with AI agentsNativeScript/runtime/ErrorEvents.cpp (1)75-78: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Fallback branch receives swapped arguments.
Line 63 calls extendStaticFunctions(child, parent). The first two candidates accept (child, parent). The third candidate, assignPropertiesFromParentToChild, is declared (parent, child) at line 84, so it would copy static properties from the child onto the parent. Object.setPrototypeOf is always present in V8, so the branch is unreachable today. Wrap it to keep the order correct if the chain ever falls through.
🐛 Proposed fix🤖 Prompt for AI Agentsvar extendStaticFunctions = Object.setPrototypeOf || (hasInternalProtoProperty() && function (child, parent) { child.__proto__ = parent; }) - || assignPropertiesFromParentToChild; + || function (child, parent) { assignPropertiesFromParentToChild(parent, child); };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/js/ts-helpers.js` around lines 75 - 78, Update the fallback assignment for extendStaticFunctions so it adapts the existing (child, parent) call signature to assignPropertiesFromParentToChild’s (parent, child) parameter order, preserving static-property copying from parent to child if earlier candidates are unavailable.57-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add TryCatch diagnostics around the builtin execution.
If internal/error-events.js throws, RunBuiltin returns an empty handle and tns::Assert aborts without the JS error message or stack. Other call sites in this PR log the pending exception before failing. Apply the same treatment here to keep initialization failures debuggable.
♻️ Proposed change🤖 Prompt for AI AgentsLocal<Value> result; - success = BuiltinLoader::RunBuiltin(context, BuiltinId::kErrorEvents, binding) - .ToLocal(&result); + { + TryCatch tc(isolate); + success = + BuiltinLoader::RunBuiltin(context, BuiltinId::kErrorEvents, binding) + .ToLocal(&result); + if (!success && tc.HasCaught()) { + tns::LogError(isolate, tc); + } + } tns::Assert(success && result->IsArray(), isolate);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/ErrorEvents.cpp` around lines 57 - 60, Update the ErrorEvents initialization around BuiltinLoader::RunBuiltin to use a v8::TryCatch, and log or report the pending JavaScript exception with its message and stack before the existing assertion when execution fails. Preserve the current result array validation and success path.
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 `@NativeScript/runtime/js/blob-url.js`: - Around line 27-59: Update the URL.prototype.searchParams getter to track the query string used to construct the cached URLSearchParams and rebuild it whenever the current search value changes, including after search or href assignments. Define _searchParams as a non-enumerable property while preserving the existing mutation write-back behavior. --- Nitpick comments: In `@NativeScript/runtime/ErrorEvents.cpp`: - Around line 57-60: Update the ErrorEvents initialization around BuiltinLoader::RunBuiltin to use a v8::TryCatch, and log or report the pending JavaScript exception with its message and stack before the existing assertion when execution fails. Preserve the current result array validation and success path. In `@NativeScript/runtime/js/ts-helpers.js`: - Around line 75-78: Update the fallback assignment for extendStaticFunctions so it adapts the existing (child, parent) call signature to assignPropertiesFromParentToChild’s (parent, child) parameter order, preserving static-property copying from parent to child if earlier candidates are unavailable.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 515c941e-ae65-4f25-96bc-d4a1e0671966
📥 CommitsReviewing files that changed from the base of the PR and between 4b29896 and 7590d8a.
📒 Files selected for processing (34)
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
What
Moves the ~26KB of JavaScript that was embedded as C++ string literals into real, version-controlled .js files under NativeScript/runtime/js/, compiled into the framework at build time (Node-style js2c), and adds two steady-state perf fixes for jitless mode.
Rebased onto main after the V8 14.9.207.39 upgrade (#412); originally stacked on #409.
Extraction & build
Loader
Perf (jitless steady-state)
Behavior notes
Testing
Summary by CodeRabbit
New Features
Bug Fixes / Performance