Skip to content

feat: ESM resolver hardening, HTTP module loader, ns:module dev surface - #383

Open
NathanWalker wants to merge 28 commits into
mainfrom
feat/hmr-dev-sessions
Open

feat: ESM resolver hardening, HTTP module loader, ns:module dev surface#383
NathanWalker wants to merge 28 commits into
mainfrom
feat/hmr-dev-sessions

Conversation

@NathanWalker

@NathanWalker NathanWalker commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Framework-agnostic hot module replacement on iOS with native ES modules: the device fetches modules over HTTP from the Vite dev server and applies hot updates without restarting the process.

The runtime's entire dev surface is one builtin module — ns:module — resolved through the same ns: registry as ns:util (#418): require("ns:module"), static import, and import() all yield the same frozen per-realm module, materialized lazily on first resolution. The dev surface defines no globals. Five primitives, each traceable to a V8-embedder or OS constraint:

Member Why it must be native
configureLoader(config) Import map, volatile URL patterns, and the canonicalization vocabulary, consumed inside V8's synchronous ResolveModuleCallback. The sole channel by which server/framework URL policy enters the runtime — native code carries no URL vocabulary of its own
invalidateModules(urls) Drops host-owned v8::Module records and arms a CFNetwork cache-bust nonce
getLoadedModuleUrls() Registry introspection for the JS full-reload path
createRequire(filenameOrURL) Mints a require bound to a base directory with the strict evaluation policy — policy selection and the require factory are embedder machinery
createPumpingRequire(filenameOrURL) Same, but async graphs are driven to settlement on nestable v8 platform tasks — only the embedder can pump its own event loop

A new node:module builtin re-exports createRequire only, so import { createRequire } from 'node:module' works unmodified (no .d.ts is shipped for it — @types/node already declares that ambient module). Debug builds also carry canonicalizeHttpUrlKey(url), a pure test diagnostic. Missing members are simply absent — never present-but-throwing — so feature checks work.

Async module-graph pipeline

HTTP module loads run a three-phase pipeline (StartAsyncHttpModuleGraphLoad): bodies fetch concurrently on NSURLSession background queues while the graph is discovered via ScriptCompiler::CompileModule + GetModuleRequests(); instantiation then runs with a lookup-only synchronous ResolveModuleCallback; evaluation is promise-chained under top-level await. The runtime fetches exactly the requested graph — concurrent per-module fetches overlap the dev server's transform work with on-device compile. A synchronous fetch (HttpFetchText) remains as the resolver's fallback for URLs the walk did not cover. Boot pressure is answered at the source: the dev server pre-bundles @nativescript/core and node_modules into single-eval payloads, and the pipeline fetches the remaining app graph concurrently.

Module identity & freshness

Module identity is the canonical URL: the server emits exactly one URL per module and never varies it for freshness (this closes the realm-split / Cannot redefine property crash class). Canonicalization survives only to absorb externally-caused variance (Vite's ?v=/?import/?t= markers, file://http:// wrapping); the mechanism (fragment strip, param drop, sort) is native, while the vocabulary (which params to strip, which path prefixes are dev endpoints, which paths keep their query verbatim) is supplied by the client via configureLoader. Freshness is explicit eviction at both layers that could serve a stale byte: the V8 module registry, and a one-shot __ns_dev_nonce that defeats CFNetwork's cache (observed serving stale bodies on iOS 18+/26 Sim despite no-store and a zero-capacity NSURLCache).

Loader architecture

Disk and HTTP ES modules now share one architecture, aligned with the invariants Node and Blink both obey: the resolver only ever compiles and registers — it never instantiates or evaluates — so V8 drives graph discovery during the root's InstantiateModule, evaluation happens once at the root in spec order, and import cycles terminate through the registry the way module-map self-inserts do elsewhere. The re-entry/fallback/gating machinery that previously compensated for resolver-order evaluation is deleted. Per-isolate loader state (registry, in-flight bookkeeping, waiters, async graph loads) lives in a typed Caches state slot destroyed with the isolate, replacing process-wide isolate-keyed maps; module→key reverse lookups (referrer discovery, import.meta) go through an identity-hash index instead of registry scans. Boot state is derived natively — the cold-boot runloop pump is armed only while an entry module is evaluating on that thread — which is why setDevBootComplete no longer exists, and the module-graph waits share one deadline constant (transport < settle < boot backstop, by construction).

Module evaluation modes & require(esm)

Graph evaluation is one primitive, EvaluateModuleGraph, under three named policies:

  • kSyncStrict — Node's require(esm): IsGraphAsync() is refused before evaluation ever starts (the module stays instantiated and loadable via import(), and the refusal wins over any runtime error the graph would have produced), including on registry hits already at kEvaluated — a TLA-parked graph reports kEvaluated with a pending capability promise, so returning its namespace would hand require() a TDZ binding. Sync graphs must settle synchronously; the promise state is read directly.
  • kSyncPumping — the boot/worker-entry settle loop: nestable v8 tasks + microtask checkpoints, HTTP-only runloop slices, 1s local yield window vs 60s HTTP deadline.
  • kAsync — evaluate and hand back the capability promise (the dynamic-import shape).

require() of an ES module is now Node-strict (previously it fell into the pumping path by accident): a graph containing top-level await throws, matching ERR_REQUIRE_ASYNC_MODULE. Its exports follow Node's populateCJSExportsFromESM exactly — a literal 'module.exports' export wins; a namespace with no default or its own __esModule passes through; otherwise a synthetic facade module re-exports the target with live bindings and __esModule = true, so transpiled consumers (_mod.__esModule ? _mod.default : _mod) find the real default. createPumpingRequire is the explicit escape hatch for native-boundary calls: async graphs are driven to settlement (60s deadline, throws rather than returning a half-initialized namespace) without ever advancing the Cocoa runloop — outside boot the runloop belongs to the app. Also fixes require("~foo"), which previously lost its first letter to an unconditional substr(2).

Additional

  • ESM resolver hardening (ModuleInternalCallbacks.mm): HTTP(S) URLs end-to-end (resolve, fetch, dynamic import) and .json imports compiled into synthetic ES modules. Builtin specifiers (ns:, registered node:) resolve only through the registry — an import-map entry can never shadow them onto HTTP.
  • Worker correctness: module registries keyed by v8::Isolate* (not thread-locals); worker teardown preserves the main isolate's process-wide dev state; worker entry-script errors propagate to worker.onerror.
  • Security: every remote fetch — the sync fallback and the async graph walk alike — passes IsRemoteUrlAllowed() (DevFlags.mm): deny-by-default in release, opt-in via security.allowRemoteModules (+ optional remoteModuleAllowlist).
  • On-device tests (TestRunner/app/tests/HttpEsmLoaderTests.js, EsmInteropTests.js, CreateRequireTests.js) pin the ns:module module shape, the require/import() identity, the exports-interop cascade, and the strict/pumping differential.

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR updates the runtime’s HTTP module loading and HMR dev-session plumbing, adds per-isolate module registry handling, and hardens the CI test harness. It also expands tests for HTTP ESM loading, import maps, blob modules, and remote module security.

Changes

Runtime: HMR & HTTP module system

Layer / File(s) Summary
HTTP dev flags & URL allowlist security
NativeScript/runtime/DevFlags.h, NativeScript/runtime/DevFlags.mm
Adds IsHttpFetchUrlLogEnabled() and changes remote-module allowlist matching to boundary-aware URL entry matching.
HMR support API surface expansion
NativeScript/runtime/HMRSupport.h, NativeScript/runtime/HMRSupport.mm
Adds HTTP fetch caching, prewarm, cache-busting, boot gating, cleanup, and __NS_DEV__ wiring for the HMR dev surface.
Per-isolate module registry & import-map API
NativeScript/runtime/ModuleInternalCallbacks.h
Replaces the global module registry with per-isolate access and adds module lifecycle, invalidation, diagnostics, and import-map APIs.
RunModule signature & Runtime teardown/init
NativeScript/runtime/ModuleInternal.h, NativeScript/runtime/Runtime.h, NativeScript/runtime/Runtime.mm, NativeScript/runtime/ModuleInternal.mm
Changes RunModule to return bool with optional error text, rewires runtime teardown/init for isolate-scoped cleanup, updates import.meta.url/dirname, and overhauls module loading/error handling.
Blob URL polyfill & HMR worker termination
NativeScript/runtime/URLImpl.cpp, NativeScript/runtime/URLImpl.h, NativeScript/runtime/Worker.h, NativeScript/runtime/Worker.mm, NativeScript/runtime/WorkerWrapper.mm
Adds blob URL creation/revocation support, URL searchParams synchronization, a dev callback that terminates all workers, and a guard for worker task draining.

CI, test harness & test coverage

Layer / File(s) Summary
CI workflow: macOS 15, Xcode 26, diagnostics
.github/workflows/npm_release.yml
Updates the test job runner and adds failure-only diagnostics artifact collection.
Embassy server: remove fatal errors & bounded shutdown
TestRunnerTests/Embassy/DefaultHTTPServer.swift, TestRunnerTests/Embassy/TCPSocket.swift, TestRunnerTests/Embassy/Transport.swift
Replaces fatal traps and unbounded waits with graceful teardown and bounded shutdown behavior.
Test runner quarantine, progress tracking & server routes
TestRunnerTests/TestRunnerTests.swift, TestRunner.app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js, TestRunnerTests/QUARANTINED_TESTS.md
Adds progress reporting, local test routes, Jasmine failure helpers, spec quarantining, and quarantine documentation.
HTTP ESM, import-map, blob & security test coverage
TestRunner/app/tests/HttpEsmLoaderTests.js, TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs, TestRunner/app/tests/RemoteModuleSecurityTests.js, TestRunner/app/tests/MethodCallsTests.js
Updates loader, import-map, blob-module, remote-security, and optional-property tests to match the runtime and harness changes.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Runtime
  participant HMRSupport
  participant ModuleInternal
  participant Worker
  Runtime->>HMRSupport: InitializeHmrDevGlobals(isolate, context, isWorker)
  HMRSupport->>HMRSupport: kickstartPrefetch, setDevBootComplete
  Runtime->>ModuleInternal: RunModule(path, outErrorMessage)
  ModuleInternal->>ModuleInternal: LoadHttpModuleForUrl / LoadESModule
  ModuleInternal-->>Runtime: bool + optional error
  Runtime->>HMRSupport: CleanupHMRGlobals (main isolate only)
  Runtime->>Worker: __NS_DEV__.terminateAllWorkers
Loading

Possibly related issues

Possibly related PRs

  • NativeScript/ios#375: Both PRs modify NativeScript/runtime/ModuleInternal.mm and the same module-loading error paths, especially debug-mode propagation and failure handling.

Poem

🐇 I hopped through URLs, swift and bright,
Cached the dawn and trimmed the night.
Workers bowed and logs took flight,
New modules loaded just right.
Carrot-powered tests all passed tonight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.16% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: ESM resolver hardening, HTTP module loading, and the new development surface.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread NativeScript/runtime/URLImpl.cpp Outdated
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch from 8310eac to 2c5d877 Compare June 17, 2026 00:01
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch 2 times, most recently from 6dfbacd to f7cdfcc Compare June 26, 2026 17:56
@edusperoni

Copy link
Copy Markdown
Collaborator

I'm not sure I understand the goal of these "dev sessions"? What do these need specifically from the runtime that's not an "user land" thing?

@NathanWalker

NathanWalker commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure I understand the goal of these "dev sessions"? What do these need specifically from the runtime that's not an "user land" thing?

Yeah good question @edusperoni, the "dev session" naming here probably over (or mis) characterizes things.

The "session" part is just the contract for booting from a dev server over HTTP instead of the on-disk bundle. It does three things a one-shot bundle loader doesnt: point resolution at an HTTP origin + install the import map before the first import, give a re-entrant boot (import client > import entry) that can re-run for a full reload without relaunching the process, and bubble import failures back as a rejected promise so the client can show an overlay instead of the app just dying.

Could it be userland? I think the thing that trips people up (tripped me up too) is that on the web HMR is userland because the browser is the runtime; it already ships a spec ESM loader that fetches over HTTP and a host-owned module map you poke at by varying the URL. Vite's client gets to be "just JS" because it sits on top of that. Here V8 is embedded by us, and bare V8 ships no loader at all; every piece of it is an embedder host callback only native can install. So the litmus test is pretty clean: anything that has to install/drive a V8 host callback or mutate V8's module map cant be userland, everything else stays in JS.

This may help expand a few things:

  • The HTTP ESM loader, import 'http://...', is resolved inside ResolveModuleCallback / the dynamic-import callback, before any user code for that module even runs. And in V8 10.3.22 that static callback is synchronous, which is the whole reason the prefetch engine exists (issue with V8's sync walk from JS, and a serial network walk on the UI thread trips the launch watchdog; an iOS issue I sort of knew about long ago so was interesting to be reminded of here).
  • Identity-preserving eviction (__nsInvalidateModules), the registry is host-owned (g_moduleRegistry) and theres no JS API to evict/re-instantiate a compiled record. This is the most NS-specific one: the web just mints a new identity per save with ?t=, but we cant, because module identity is load-bearing for native interop - mint a fresh realm for a @nativescript/core module and the native patches collide leading to Cannot redefine property. So the runtime has to own eviction that collapses back to one canonical identity.
  • import.meta.hot: import.meta only gets populated in SetHostInitializeImportMetaObjectCallback, and hot.data only survives a swap if it's keyed to the runtime's canonical module key, which userland doesnt have.
  • Reboot teardown: terminating native-owned workers + ordered v8::Global/thread-local cleanup before isolate disposal.
    Everything protocol-ish I deliberately left in JS: the WebSocket, the wire format, evictPaths/closure computation, accept/dispose policy; all in @nativescript/vite's client. Runtime exposes mechanism, client owns policy; nothing links or version-pins vite, and unknown URLs just fall through to a generic HTTP loader.

So really these globals arent "a dev-session feature" so much as the embedder half of a spec ESM loader + an identity-preserving module map. The part the browser hands Vite for free. There is likely a few that could move to JS if we want a smaller surface like __nsApplyStyleUpdate (just Application.addCss + restyle) and __nsGetLoadedModuleUrls (introspection) and others. Lmk if you see the boundary differently and we can make further adjustments.

@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch from d24c897 to 4289539 Compare June 27, 2026 20:12
@NathanWalker
NathanWalker marked this pull request as ready for review June 29, 2026 21:39

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
NativeScript/runtime/Runtime.mm (1)

445-458: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Honor the new RunModule failure contract in main startup.

Line 450 still discards the boolean result. Since ModuleInternal::RunModule now reports some failures by returning false without throwing, startup can continue after the main module failed.

Proposed fix
 void Runtime::RunMainScript() {
   Isolate* isolate = this->GetIsolate();
   v8::Locker locker(isolate);
   Isolate::Scope isolate_scope(isolate);
   HandleScope handle_scope(isolate);
-  this->moduleInternal_->RunModule(isolate, "./");
+  std::string err;
+  if (!this->moduleInternal_->RunModule(isolate, "./", &err)) {
+    throw NativeScriptException(
+        isolate,
+        err.empty() ? "Failed to run main module" : err,
+        "Error");
+  }
 }
🤖 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/Runtime.mm` around lines 445 - 458,
Runtime::RunMainScript currently ignores the boolean result from
ModuleInternal::RunModule, so startup can continue even when main module loading
fails without throwing. Update RunMainScript to use the same failure contract as
Runtime::RunModule by capturing the return value from moduleInternal_->RunModule
and handling a false result as a startup failure, using the existing Runtime and
ModuleInternal::RunModule symbols to locate the change.
🧹 Nitpick comments (4)
NativeScript/runtime/ModuleInternalCallbacks.h (1)

44-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Keep loaded-module introspection isolate-explicit.

Now that the registry is keyed by v8::Isolate*, GetLoadedModuleUrls() should take the target isolate like the other registry APIs. Relying on an implicit current isolate makes worker/main diagnostics easier to mix up.

Suggested API adjustment
-std::vector<std::string> GetLoadedModuleUrls();
+std::vector<std::string> GetLoadedModuleUrls(v8::Isolate* isolate);
🤖 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/ModuleInternalCallbacks.h` around lines 44 - 45,
GetLoadedModuleUrls() is still using an implicit current isolate, which can mix
up worker and main-thread diagnostics now that the module registry is
isolate-keyed. Update the ModuleInternalCallbacks API so GetLoadedModuleUrls
takes a v8::Isolate* parameter, and propagate that isolate through the
implementation and any call sites to match the other registry helpers. Use the
existing registry symbols in ModuleInternalCallbacks to keep the diagnostics
explicitly scoped to the target isolate.
.github/workflows/npm_release.yml (1)

263-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Bound the failure diagnostics payload.

Copying the full CoreSimulator log tree plus an unrestricted log collect can make failed CI runs slow or produce oversized artifacts. Prefer a targeted logarchive window and avoid uploading the whole CoreSimulator directory.

Suggested tightening
           # Simulator app crashes land in the host's DiagnosticReports.
           cp -R ~/Library/Logs/DiagnosticReports/. "$DIAG/DiagnosticReports/" 2>/dev/null || true
-          cp -R ~/Library/Logs/CoreSimulator/. "$DIAG/CoreSimulator/" 2>/dev/null || true
+          # Avoid uploading the full CoreSimulator log tree; the targeted
+          # logarchive below contains the simulator logs needed for this run.
@@
-            xcrun simctl spawn "$UDID" log collect --output "$DIAG/simulator.logarchive" 2>/dev/null || true
+            xcrun simctl spawn "$UDID" log collect --last 45m --output "$DIAG/simulator.logarchive" 2>/dev/null || true

Please verify the log collect --last option on the macOS 15/Xcode 26 runner image.

🤖 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 @.github/workflows/npm_release.yml around lines 263 - 279, The failure
diagnostics step is too broad: it copies the entire CoreSimulator log tree and
runs an unbounded log collect, which can create oversized artifacts and slow CI.
In the diagnostics block that uses DIAG, xcrun simctl spawn, and log collect,
stop archiving the full CoreSimulator directory and switch to a targeted
unified-log collection window using the macOS 15/Xcode 26-supported log collect
--last option so only recent logs are captured.
NativeScript/runtime/Runtime.mm (1)

252-253: 🩺 Stability & Availability | 🔵 Trivial

Track the worker queue race TODO.

This TODO names a possible worker queue leak during termination ordering. Please track it before merge or file a follow-up so it does not get lost. I can help draft the issue or a fix plan.

🤖 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/Runtime.mm` around lines 252 - 253, The TODO in
Runtime.mm about a possible worker queue leak/race during termination ordering
needs to be tracked before merge. Follow up on the worker lifecycle path in the
Runtime-related initialization/termination flow, especially around the queue
handling and the Terminate-before-Initialize scenario, and either replace the
TODO with a concrete fix or create a tracked issue/fix plan linked to the worker
queue race so it is not lost.
NativeScript/runtime/URLImpl.cpp (1)

59-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Keep searchParams synchronized after url.search changes.

The getter returns the cached _searchParams forever. If code reads url.searchParams, then later assigns url.search, subsequent url.searchParams reads still expose the old query.

🤖 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/URLImpl.cpp` around lines 59 - 90, The
URLImpl::searchParams getter caches a single _searchParams instance and never
refreshes it when url.search changes, so later reads can return stale query
data. Update the URL.prototype.searchParams handling in URLImpl.cpp so the
cached URLSearchParams is invalidated or resynced whenever the search setter
runs, and make sure the getter recreates/updates the instance from the current
search string before returning it.
🤖 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 `@NativeScript/runtime/DevFlags.mm`:
- Around line 95-103: The allowlist check in RemoteUrlMatchesAllowlistEntry is
matching raw URL prefixes too early, which lets path-scoped entries be bypassed
with dot-segment paths. Update the matching logic to canonicalize or normalize
the URL path before applying the prefix/boundary rules, or explicitly reject
plain/encoded dot segments in DevFlags.mm so a trailing-slash allowlist entry
cannot match escaped paths.

In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 236-264: The debug handling in ModuleInternal.mm is swallowing
non-HTTP ES module failures by returning true or an empty namespace, which
prevents worker error propagation for .mjs loads. Update the
NativeScriptException catch and the moduleNamespace.IsEmpty path so debug mode
still surfaces failures to the caller for worker-loaded ESM, instead of always
pretending success; keep the existing HTTP debug logging, but ensure the
Worker.mm TryCatch can observe the error for the ES module load path.

In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 94-100: The install script execution in URLImpl should not fail
silently: the current Compile/Run flow can leave blob URL support partially
initialized and a pending V8 exception uncleared. Update the script path in
URLImpl to wrap the v8::Script::Compile and script->Run calls in a v8::TryCatch,
then explicitly handle both compile and runtime failures by logging the error
and propagating it (or throwing) instead of ignoring the result. Use the
existing blob_methods script setup in URLImpl as the place to add this error
handling.

In `@NativeScript/runtime/Worker.mm`:
- Around line 486-499: The HMR termination loop in Worker::TerminateWorkers
currently iterates all entries from Caches::Workers, which can affect workers
from other runtimes. Update this callback to filter worker wrappers by the
current main isolate, matching the existing Runtime::~Runtime() behavior via
WorkerWrapper::GetMainIsolate(). Keep the existing running/closing checks and
only call WorkerWrapper::Terminate() for workers belonging to the same isolate.

In `@TestRunner/app/tests/esm/hmr/hot-data-ext.js`:
- Around line 51-73: The hot-data fixture is mutating shared HMR state by
invoking hot.accept, hot.dispose, hot.decline, and hot.invalidate in the shared
test helper, which makes later specs order-dependent. Update hot-data-ext.js so
the shared fixture only checks for the presence of HMR APIs and data on hot, and
remove lifecycle/callback registration from this path. If coverage for those
methods is needed, move it into a separate throwaway module or dedicated test
helper that is not reused across specs.

In `@TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs`:
- Around line 55-78: The test cleanup in the __ns_test_vendor__ import-map spec
is not restoring the runtime’s prior import-map state, which can leak
configuration into later specs. Snapshot the existing import-map before calling
configureRuntime in this test, then in the finally block restore that original
import-map instead of resetting to an empty imports object. Keep the existing
__nsVendorRegistry restore logic intact so the test remains hermetic.

In `@TestRunnerTests/Embassy/TCPSocket.swift`:
- Around line 60-66: The SO_NOSIGPIPE setup in TCPSocket is currently ignoring
the result of setsockopt, which can leave the socket in a state where send may
trigger SIGPIPE before Transport.handleWrite() can observe EPIPE. Update the
socket setup path in the TCPSocket initializer/helper to check the setsockopt
return value, and if it fails on Darwin, immediately treat it as a socket error
by closing the socket and propagating an error back to the caller instead of
discarding it. Use the TCPSocket and Transport.handleWrite symbols to locate the
write-path setup and keep the failure handling aligned with the existing socket
lifecycle.

In `@TestRunnerTests/TestRunnerTests.swift`:
- Line 24: The test setup in DefaultHTTPServer is still binding the server to
127.0.0.1 even though TCPSocket.bind(interface:) currently treats the interface
as IPv6, so the listener is not truly IPv4. Update the TestRunnerTests/server
setup to use the matching loopback family consistently (for example, switch both
server and client-side expectations to ::1/[::1]), or if IPv4 is required,
adjust TCPSocket and the DefaultHTTPServer path to support AF_INET first. Use
the existing DefaultHTTPServer initializer and TCPSocket.bind interface handling
to locate the change.

---

Outside diff comments:
In `@NativeScript/runtime/Runtime.mm`:
- Around line 445-458: Runtime::RunMainScript currently ignores the boolean
result from ModuleInternal::RunModule, so startup can continue even when main
module loading fails without throwing. Update RunMainScript to use the same
failure contract as Runtime::RunModule by capturing the return value from
moduleInternal_->RunModule and handling a false result as a startup failure,
using the existing Runtime and ModuleInternal::RunModule symbols to locate the
change.

---

Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 263-279: The failure diagnostics step is too broad: it copies the
entire CoreSimulator log tree and runs an unbounded log collect, which can
create oversized artifacts and slow CI. In the diagnostics block that uses DIAG,
xcrun simctl spawn, and log collect, stop archiving the full CoreSimulator
directory and switch to a targeted unified-log collection window using the macOS
15/Xcode 26-supported log collect --last option so only recent logs are
captured.

In `@NativeScript/runtime/ModuleInternalCallbacks.h`:
- Around line 44-45: GetLoadedModuleUrls() is still using an implicit current
isolate, which can mix up worker and main-thread diagnostics now that the module
registry is isolate-keyed. Update the ModuleInternalCallbacks API so
GetLoadedModuleUrls takes a v8::Isolate* parameter, and propagate that isolate
through the implementation and any call sites to match the other registry
helpers. Use the existing registry symbols in ModuleInternalCallbacks to keep
the diagnostics explicitly scoped to the target isolate.

In `@NativeScript/runtime/Runtime.mm`:
- Around line 252-253: The TODO in Runtime.mm about a possible worker queue
leak/race during termination ordering needs to be tracked before merge. Follow
up on the worker lifecycle path in the Runtime-related
initialization/termination flow, especially around the queue handling and the
Terminate-before-Initialize scenario, and either replace the TODO with a
concrete fix or create a tracked issue/fix plan linked to the worker queue race
so it is not lost.

In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 59-90: The URLImpl::searchParams getter caches a single
_searchParams instance and never refreshes it when url.search changes, so later
reads can return stale query data. Update the URL.prototype.searchParams
handling in URLImpl.cpp so the cached URLSearchParams is invalidated or resynced
whenever the search setter runs, and make sure the getter recreates/updates the
instance from the current search string before returning it.
🪄 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

Run ID: 5096dcca-ed10-47ea-be7c-64aafa7275ac

📥 Commits

Reviewing files that changed from the base of the PR and between e652dd0 and ada922f.

📒 Files selected for processing (28)
  • .github/scripts/sample-hung-app.sh
  • .github/workflows/npm_release.yml
  • NativeScript/runtime/DevFlags.h
  • NativeScript/runtime/DevFlags.mm
  • NativeScript/runtime/HMRSupport.h
  • NativeScript/runtime/HMRSupport.mm
  • NativeScript/runtime/ModuleInternal.h
  • NativeScript/runtime/ModuleInternal.mm
  • NativeScript/runtime/ModuleInternalCallbacks.h
  • NativeScript/runtime/ModuleInternalCallbacks.mm
  • NativeScript/runtime/Runtime.h
  • NativeScript/runtime/Runtime.mm
  • NativeScript/runtime/URLImpl.cpp
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/Worker.h
  • NativeScript/runtime/Worker.mm
  • TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js
  • TestRunner/app/tests/HttpEsmLoaderTests.js
  • TestRunner/app/tests/MethodCallsTests.js
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunner/app/tests/RemoteModuleSecurityTests.js
  • TestRunner/app/tests/esm/hmr/hot-data-ext.js
  • TestRunner/app/tests/esm/hmr/hot-data-ext.mjs
  • TestRunnerTests/Embassy/DefaultHTTPServer.swift
  • TestRunnerTests/Embassy/TCPSocket.swift
  • TestRunnerTests/Embassy/Transport.swift
  • TestRunnerTests/QUARANTINED_TESTS.md
  • TestRunnerTests/TestRunnerTests.swift

Comment thread NativeScript/runtime/DevFlags.mm Outdated
Comment thread NativeScript/runtime/ModuleInternal.mm Outdated
Comment thread NativeScript/runtime/URLImpl.cpp Outdated
Comment thread NativeScript/runtime/Worker.mm Outdated
Comment thread TestRunner/app/tests/esm/hmr/hot-data-ext.js Outdated
Comment thread TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
Comment thread TestRunnerTests/Embassy/TCPSocket.swift Outdated
Comment thread TestRunnerTests/TestRunnerTests.swift Outdated
@NathanWalker NathanWalker changed the title feat: HMR dev-sessions, ESM resolver hardening, dev-mode runtime globals feat: ESM resolver hardening, http loader, dev-mode globals Jul 3, 2026
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch from ada922f to c81143d Compare July 3, 2026 19:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
NativeScript/runtime/Worker.mm (1)

228-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle RunModule’s new failure return in workers.

Runtime::RunModule now reports load/evaluation failures via false plus outErrorMessage. This worker path still ignores the return value and only checks TryCatch, so non-thrown HTTP ESM/TLA failures may never reach worker.onerror.

Proposed fix
-      runtime->RunModule(resolvedPath);
+      std::string errorMessage;
+      bool didRun = runtime->RunModule(resolvedPath, &errorMessage);
+      if (!didRun && !tc.HasCaught()) {
+        worker->PassUncaughtExceptionFromWorkerToMain(
+            errorMessage.empty() ? "Worker script failed: " + resolvedPath : errorMessage,
+            resolvedPath, "", 1, true);
+        worker->Terminate();
+        return isolate;
+      }
🤖 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/Worker.mm` around lines 228 - 229, The worker module
loading path in Worker::Start still ignores Runtime::RunModule’s new false
return, so failures that only populate outErrorMessage never surface to
worker.onerror. Update the RunModule call site to capture the boolean result and
error message, then route that failure through the same worker error handling
path used for TryCatch so both thrown and non-thrown evaluation/load errors are
reported consistently.
🧹 Nitpick comments (4)
TestRunner/app/tests/HttpEsmLoaderTests.js (2)

245-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead code after pending() calls.

pending() called synchronously in a spec body throws internally and halts execution immediately, so the following done(); return; lines never run. Harmless but misleading; can be dropped for clarity.

♻️ Simplify skip guards
             if (!origin) {
                 pending("REPORT_BASEURL not set; skipping host HTTP tests");
-                done();
-                return;
             }

Also applies to: 271-275, 298-302, 323-327

🤖 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/HttpEsmLoaderTests.js` around lines 245 - 249, The skip
guards in the HTTP ESM loader specs contain dead code after synchronous
pending() calls. In the relevant test blocks within HttpEsmLoaderTests.js,
remove the unnecessary done(); return; lines that follow pending() so the guard
reads cleanly and matches the actual control flow; apply the same simplification
to the other pending() skip checks in the same test file.

242-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Repeated skip-guard boilerplate across canonicalization tests.

The if (!origin) { pending(...); done(); return; } block is duplicated across all four tests in this suite. Consider extracting a small helper (e.g. requireHostOriginOrSkip(done)) alongside the existing formatError/withTimeout/getHostOrigin helpers.

🤖 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/HttpEsmLoaderTests.js` around lines 242 - 344, The URL
key canonicalization tests repeat the same host-skip guard in each case, making
the suite noisy and harder to maintain. Add a small shared helper near the
existing getHostOrigin, withTimeout, and formatError utilities (for example, a
function like requireHostOriginOrSkip(done)) that checks for a missing origin,
calls pending with the same message, then finishes the test early. Update each
of the four describe("URL Key Canonicalization") specs to use that helper
instead of duplicating the if (!origin) block.
NativeScript/runtime/HMRSupport.mm (1)

8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Include <memory> directly for std::shared_ptr and std::make_shared.

This file uses std::shared_ptr and std::make_shared later, but the changed include list does not include <memory>, so it relies on transitive includes.

Proposed fix
 `#include` <mutex>
+#include <memory>
 `#include` "Helpers.h"
🤖 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/HMRSupport.mm` around lines 8 - 18, Add the missing
direct include for <memory> in HMRSupport.mm because the file uses
std::shared_ptr and std::make_shared and should not rely on transitive headers.
Update the include block near the top of the file so the needed standard library
types are declared explicitly, keeping the existing RuntimeConfig, Worker, and
helper includes unchanged.
.github/workflows/npm_release.yml (1)

262-279: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

UDID fallback may resolve to the wrong simulator on a multi-runtime runner.

If no device is currently booted, the fallback searches xcrun simctl list devices 'iPhone 16 Pro' across every installed runtime and takes head -1. On a runner image with multiple Xcode/runtime versions pre-installed, this can match a same-named simulator from an unrelated iOS runtime that never ran the tests, producing an essentially empty/irrelevant simulator.logarchive instead of the one from the actual failing run — undermining the stated goal of showing "the app's console output ... before a hang" (Line 262-263 comment).

♻️ Possible mitigation

Filter the fallback list by the runtime actually used for testing (e.g. match the OS=latest runtime for env.XCODE_VERSION), or persist the UDID resolved by xcodebuild -destination during the "Xcode Tests" step to a file and reuse it here instead of re-searching by name only.

🤖 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 @.github/workflows/npm_release.yml around lines 262 - 279, The UDID lookup in
the simulator log collection step can pick the wrong device on runners with
multiple runtimes because it falls back to a name-only search in xcrun simctl
list devices. Update the logarchive collection logic to reuse the exact
simulator used by the Xcode Tests step or filter the fallback by the same
runtime/destination as the test run, then keep using that resolved UDID in the
xcrun simctl spawn log collect path.
🤖 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 `@NativeScript/runtime/HMRSupport.mm`:
- Around line 806-814: Timed-out prefetch work in HMRSupport.mm can still
complete later and overwrite g_prefetchCache with stale data after
dispatch_group_wait has already returned. Add a per-prefetch generation or
cancel marker around the prefetch flow in the relevant HMRSupport functions
handling the cache write path, and check that marker immediately before storing
the fetched body. Make sure the guard is shared by the scheduled blocks in both
affected sections so only the active reload cycle can commit into
g_prefetchCache.
- Around line 36-43: `InitializeHmrDevGlobals` only defines the `globalThis`
mirror once, so repeated or re-entrant installs can leave
`globalThis.__NS_DEV__` pointing at a stale object. Update the generated
assignment in `HMRSupport.mm` so the mirror is refreshed on each install instead
of only when undefined, while still preserving the `Object.defineProperty`
behavior used for `name` and the `__NS_DEV__` global.

In `@NativeScript/runtime/Runtime.mm`:
- Around line 137-148: Update the hasUrlScheme handling in Runtime::dirname
logic so single-segment HTTP-style module URLs like http://host/main.js are
reduced to the host root for import.meta.dirname instead of keeping the full
module URL. The current lastSlash > pathStart check in the modulePath branch is
too strict; adjust the condition or split logic so the final path segment is
stripped whenever a URL path exists after the host, while still preserving the
identity for host-only or non-hierarchical schemes such as node:fs and blob:abc.

In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 61-89: The cached URLSearchParams instance in URLImpl is not
refreshed when the search string is reassigned, so stale values can be returned
after setting URL.search. Update the SetSearch logic in URLImpl to either clear
_searchParams when search changes or synchronize the existing object with the
new query string, so later URL.searchParams access reflects the latest value.
- Around line 26-91: The injected blob URL setup in URLImpl.cpp is not
idempotent: repeated execution can redeclare BLOB_STORE and InternalAccessor and
can fail when redefining URL.prototype.searchParams. Update the blob_methods
script so its top-level declarations are guarded or reused on subsequent
installs, and make the searchParams property definition configurable so the
accessor can be safely reinstalled without throwing. Use the existing
URL.createObjectURL, URL.revokeObjectURL, InternalAccessor, and
Object.defineProperty(URL.prototype, 'searchParams', ...) sections as the fix
points.

---

Outside diff comments:
In `@NativeScript/runtime/Worker.mm`:
- Around line 228-229: The worker module loading path in Worker::Start still
ignores Runtime::RunModule’s new false return, so failures that only populate
outErrorMessage never surface to worker.onerror. Update the RunModule call site
to capture the boolean result and error message, then route that failure through
the same worker error handling path used for TryCatch so both thrown and
non-thrown evaluation/load errors are reported consistently.

---

Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 262-279: The UDID lookup in the simulator log collection step can
pick the wrong device on runners with multiple runtimes because it falls back to
a name-only search in xcrun simctl list devices. Update the logarchive
collection logic to reuse the exact simulator used by the Xcode Tests step or
filter the fallback by the same runtime/destination as the test run, then keep
using that resolved UDID in the xcrun simctl spawn log collect path.

In `@NativeScript/runtime/HMRSupport.mm`:
- Around line 8-18: Add the missing direct include for <memory> in HMRSupport.mm
because the file uses std::shared_ptr and std::make_shared and should not rely
on transitive headers. Update the include block near the top of the file so the
needed standard library types are declared explicitly, keeping the existing
RuntimeConfig, Worker, and helper includes unchanged.

In `@TestRunner/app/tests/HttpEsmLoaderTests.js`:
- Around line 245-249: The skip guards in the HTTP ESM loader specs contain dead
code after synchronous pending() calls. In the relevant test blocks within
HttpEsmLoaderTests.js, remove the unnecessary done(); return; lines that follow
pending() so the guard reads cleanly and matches the actual control flow; apply
the same simplification to the other pending() skip checks in the same test
file.
- Around line 242-344: The URL key canonicalization tests repeat the same
host-skip guard in each case, making the suite noisy and harder to maintain. Add
a small shared helper near the existing getHostOrigin, withTimeout, and
formatError utilities (for example, a function like
requireHostOriginOrSkip(done)) that checks for a missing origin, calls pending
with the same message, then finishes the test early. Update each of the four
describe("URL Key Canonicalization") specs to use that helper instead of
duplicating the if (!origin) block.
🪄 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

Run ID: db3ffc7d-0cdb-488b-a6db-465fc500c772

📥 Commits

Reviewing files that changed from the base of the PR and between ada922f and c81143d.

📒 Files selected for processing (26)
  • .github/scripts/sample-hung-app.sh
  • .github/workflows/npm_release.yml
  • NativeScript/runtime/DevFlags.h
  • NativeScript/runtime/DevFlags.mm
  • NativeScript/runtime/HMRSupport.h
  • NativeScript/runtime/HMRSupport.mm
  • NativeScript/runtime/ModuleInternal.h
  • NativeScript/runtime/ModuleInternal.mm
  • NativeScript/runtime/ModuleInternalCallbacks.h
  • NativeScript/runtime/ModuleInternalCallbacks.mm
  • NativeScript/runtime/Runtime.h
  • NativeScript/runtime/Runtime.mm
  • NativeScript/runtime/URLImpl.cpp
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/Worker.h
  • NativeScript/runtime/Worker.mm
  • TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js
  • TestRunner/app/tests/HttpEsmLoaderTests.js
  • TestRunner/app/tests/MethodCallsTests.js
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunner/app/tests/RemoteModuleSecurityTests.js
  • TestRunnerTests/Embassy/DefaultHTTPServer.swift
  • TestRunnerTests/Embassy/TCPSocket.swift
  • TestRunnerTests/Embassy/Transport.swift
  • TestRunnerTests/QUARANTINED_TESTS.md
  • TestRunnerTests/TestRunnerTests.swift
✅ Files skipped from review due to trivial changes (1)
  • TestRunnerTests/QUARANTINED_TESTS.md
🚧 Files skipped from review as they are similar to previous changes (15)
  • NativeScript/runtime/ModuleInternal.h
  • .github/scripts/sample-hung-app.sh
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/Worker.h
  • TestRunner/app/tests/MethodCallsTests.js
  • NativeScript/runtime/Runtime.h
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunnerTests/Embassy/TCPSocket.swift
  • TestRunner/app/tests/RemoteModuleSecurityTests.js
  • TestRunnerTests/Embassy/DefaultHTTPServer.swift
  • NativeScript/runtime/ModuleInternalCallbacks.h
  • TestRunnerTests/Embassy/Transport.swift
  • TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js
  • TestRunnerTests/TestRunnerTests.swift
  • NativeScript/runtime/ModuleInternal.mm

Comment thread NativeScript/runtime/HMRSupport.mm Outdated
Comment thread NativeScript/runtime/HMRSupport.mm Outdated
Comment thread NativeScript/runtime/Runtime.mm
Comment thread NativeScript/runtime/URLImpl.cpp Outdated
Comment thread NativeScript/runtime/URLImpl.cpp Outdated
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch from c81143d to e48b2ca Compare July 3, 2026 19:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
NativeScript/runtime/ModuleInternal.mm (1)

1031-1040: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Propagate HTTP loader failures instead of returning an empty namespace

LoadHttpModuleForUrl already throws on fetch/compile errors, but this debug branch turns that into Local<Value>(), so RunModule falls back to the generic empty-namespace message and drops the real cause. Throw here for isHttpModule so outErrorMessage carries the loader error.

🤖 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 1031 - 1040, The HTTP
module load path in ModuleInternal.mm is swallowing the real loader failure in
the isHttpModule branch by returning an empty Local<Value>() when
LoadHttpModuleForUrl fails. Update the RunModule/module compilation flow so
that, instead of returning empty in the RuntimeConfig.IsDebug branch, it
propagates the exception from LoadHttpModuleForUrl (or throws a
NativeScriptException with the loader error) and lets outErrorMessage capture
that cause. Keep the existing logPhase("compile", "fail", "http-loader") but
ensure the failure exits via an error path, not a fallback namespace return.
♻️ Duplicate comments (1)
NativeScript/runtime/ModuleInternal.mm (1)

234-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Worker .mjs ESM failures are still swallowed in debug.

The CJS require() path was updated to gate on !cache->isWorker (Line 300) and rethrows for workers (Lines 348-350), but the ESM branch here still gates only on !isHttpModule. For a .mjs worker entry, a load failure returns true (Line 240) or an empty namespace returns true (Line 256), so Worker.mm's TryCatch never observes the failure and worker.onerror never fires. This is the same concern raised previously and marked addressed — the fix appears to have landed only on the CJS path.

Proposed direction
-      if (RuntimeConfig.IsDebug && !isHttpModule) {
+      if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
         Log(@"***** JavaScript exception occurred - detailed stack trace follows *****");
         ...
         return true;  // avoid termination in debug
       } else {
         SetOutErrorMessage(outErrorMessage, ex.getMessage());
+        if (cache->isWorker && /* pending V8 exception available */) {
+          // rethrow so Worker.mm TryCatch routes to worker.onerror
+        }
         return false;
       }
@@
-      if (RuntimeConfig.IsDebug && !isHttpModule) {
+      if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
         Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded");
         return true;
🤖 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 234 - 256, The ESM
worker load path in ModuleInternal.mm still swallows failures in debug because
it only checks RuntimeConfig.IsDebug and !isHttpModule, so .mjs worker entry
errors return true instead of propagating. Update the ES module handling around
the load-failure and empty-namespace branches to also exclude worker contexts,
using the same worker-aware gating already applied in the require()/cache path,
so Worker.mm’s TryCatch can observe the exception and fire worker.onerror.
🧹 Nitpick comments (1)
.github/workflows/npm_release.yml (1)

171-176: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Update the runtime-suite note to match the actual pin. XCODE_VERSION is ^15.0, so the Xcode 26/iOS 26 comment is stale and misleading.

🤖 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 @.github/workflows/npm_release.yml around lines 171 - 176, The runtime-suite
note is stale and does not match the pinned Xcode version. Update the comment
near the workflow’s macOS runner and XCODE_VERSION pin to describe the actual
Xcode 15 / iOS 15 runtime instead of Xcode 26 / iOS 26, keeping the note
consistent with the deterministic pinning rationale in the same block.
🤖 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.

Outside diff comments:
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 1031-1040: The HTTP module load path in ModuleInternal.mm is
swallowing the real loader failure in the isHttpModule branch by returning an
empty Local<Value>() when LoadHttpModuleForUrl fails. Update the
RunModule/module compilation flow so that, instead of returning empty in the
RuntimeConfig.IsDebug branch, it propagates the exception from
LoadHttpModuleForUrl (or throws a NativeScriptException with the loader error)
and lets outErrorMessage capture that cause. Keep the existing
logPhase("compile", "fail", "http-loader") but ensure the failure exits via an
error path, not a fallback namespace return.

---

Duplicate comments:
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 234-256: The ESM worker load path in ModuleInternal.mm still
swallows failures in debug because it only checks RuntimeConfig.IsDebug and
!isHttpModule, so .mjs worker entry errors return true instead of propagating.
Update the ES module handling around the load-failure and empty-namespace
branches to also exclude worker contexts, using the same worker-aware gating
already applied in the require()/cache path, so Worker.mm’s TryCatch can observe
the exception and fire worker.onerror.

---

Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 171-176: The runtime-suite note is stale and does not match the
pinned Xcode version. Update the comment near the workflow’s macOS runner and
XCODE_VERSION pin to describe the actual Xcode 15 / iOS 15 runtime instead of
Xcode 26 / iOS 26, keeping the note consistent with the deterministic pinning
rationale in the same block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 72cb5ffd-ae59-44eb-b7be-58246e617135

📥 Commits

Reviewing files that changed from the base of the PR and between c81143d and e48b2ca.

📒 Files selected for processing (14)
  • .github/workflows/npm_release.yml
  • NativeScript/runtime/DevFlags.h
  • NativeScript/runtime/DevFlags.mm
  • NativeScript/runtime/HMRSupport.h
  • NativeScript/runtime/HMRSupport.mm
  • NativeScript/runtime/ModuleInternal.mm
  • NativeScript/runtime/ModuleInternalCallbacks.mm
  • NativeScript/runtime/Runtime.mm
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/Worker.h
  • NativeScript/runtime/Worker.mm
  • TestRunner/app/tests/HttpEsmLoaderTests.js
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunnerTests/TestRunnerTests.swift
💤 Files with no reviewable changes (6)
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/DevFlags.h
  • NativeScript/runtime/Worker.mm
  • TestRunnerTests/TestRunnerTests.swift
  • NativeScript/runtime/HMRSupport.mm
  • NativeScript/runtime/DevFlags.mm
🚧 Files skipped from review as they are similar to previous changes (5)
  • NativeScript/runtime/Worker.h
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunner/app/tests/HttpEsmLoaderTests.js
  • NativeScript/runtime/HMRSupport.h
  • NativeScript/runtime/Runtime.mm

@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch 2 times, most recently from 27efd66 to 7fd30c1 Compare July 23, 2026 19:39
@NathanWalker NathanWalker added this to the 9.1 milestone Jul 28, 2026
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch from 7fd30c1 to e4b9236 Compare July 29, 2026 21:29
@NathanWalker NathanWalker changed the title feat: ESM resolver hardening, http loader, dev-mode globals feat: ESM resolver hardening, HTTP module loader, ns:runtime dev surface Jul 30, 2026
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch 2 times, most recently from 3cd6611 to c14a5a6 Compare August 4, 2026 01:02
@NathanWalker NathanWalker changed the title feat: ESM resolver hardening, HTTP module loader, ns:runtime dev surface feat: ESM resolver hardening, HTTP module loader, ns:module dev surface Aug 10, 2026
@NathanWalker
NathanWalker force-pushed the feat/hmr-dev-sessions branch 5 times, most recently from 4a95770 to a189892 Compare August 11, 2026 22:39
@edusperoni
edusperoni force-pushed the feat/hmr-dev-sessions branch from 017dd13 to f0bb47f Compare August 18, 2026 15:16
NathanWalker and others added 8 commits August 18, 2026 13:42
Canonicalize module identity into three registry shapes — http(s) URLs,
custom schemes (node:, blob:, optional:), and absolute file paths — and
key the module registries by v8::Isolate instead of thread_local storage.
import() now rejects missing bare specifiers instead of installing
placeholders; optional-module placeholders are built without string
interpolation, detection is unified in IsLikelyOptionalModule, and module
source preserves embedded NUL bytes. Thenables handed to the loader from
JS are adopted properly. Blob URLs (blob:nativescript/<uuid>) become
first-class module identities via URL.createObjectURL and
URL.InternalAccessor.

The prewarm/prefetch machinery is replaced by an async module-graph
loader; boot hands off to a manual runloop that pumps pending module work
when the entry script has not reached UIApplicationMain yet (e.g. a
top-level-await entry still loading its graph). RunModule surfaces the
failure cause to callers through an error out-parameter.
Dev sessions serve the app's module graph over HTTP during development,
with a mechanism-only dev-loader contract: policy stays in JS tooling,
the runtime supplies fetch/registry/invalidations. The loader is
deny-by-default — remote allowlist entries only authorize URLs on a
URL-component boundary ('/', '?', '#' or exact match), refusing
lookalike-host and lookalike-port bypasses; a specific port must be
listed explicitly. Hot-path hash containers use robin_hood maps.

Per-fetch URL logging is opt-in via the httpFetchUrlLog config flag
(volume is one line per fetch), alongside the existing
logScriptLoading-gated diagnostics.
The dev-loader control surface (HMRSupport) is reachable from JS as the
ns:module builtin module: NsBuiltinModules routes ns:module through
BuildNsModuleBinding — the binding builder decides build-dependent
membership — and ns-module.js (compiled in via js2c) shapes and freezes
whatever arrives. TypeScript declarations ship in types/ns-module.d.ts,
and docs/ns-builtin-modules.md documents the surface.
Worker entry-script load failures now reach worker.onerror instead of
failing silently. Messages posted before the worker's entry script has
installed onmessage are no longer dropped: ConcurrentQueue::Signal
re-arms the drain source without enqueueing (a silent no-op when racing
Terminate), and WorkerWrapper retries delivery through a deferred drain,
with drainRetryPending_ preventing one stacked retry per attempt.
HttpEsmLoaderTests, RemoteModuleSecurityTests, and the node-builtins /
optional-modules suites exercise the async loader, allowlist boundary
matching, and the ns:module surface; the Jasmine boot shim awaits
promise-returning specs so async failures fail the run. The Embassy test
HTTP server is hardened for the loader suites, and QUARANTINED_TESTS.md
records specs excluded from the run and why.

On CI, the release workflow now collects crash reports (.ips) and the
simulator's unified log when the runtime suite fails, since the xcresult
captures nothing from inside the app.
…mposite action

Main already carries a refined port of these steps as
.github/actions/collect-test-diagnostics (#419), and after merging main the
branch ran both — two failure-time uploads of the same 'test-diagnostics'
artifact name, which upload-artifact v7 rejects. The workflow file now matches
main exactly.
@edusperoni
edusperoni force-pushed the feat/hmr-dev-sessions branch from f8b4896 to c0fb766 Compare August 18, 2026 16:43
…tate slots

One ModuleLoaderState struct, private to ModuleInternalCallbacks.mm and
reached through Caches::StateFor, now owns everything the loader keeps per
isolate: the three registry maps, the async graph-load list, the resolution
stack, the re-entry bookkeeping, and both waiter maps. This replaces the
mutex-guarded leaked process map keyed by v8::Isolate*, the async-loads
mutex + table, and six thread_local containers.

The slot is destroyed with the isolate's Caches — under the teardown Locker,
while the isolate is still alive — so the v8::Global members Reset safely in
their own destructors. That also fixes g_moduleWaiters, a process-global of
v8::Global<Promise::Resolver> that was never cleaned at teardown.
DestroyModuleStateForIsolate shrinks to QuiesceModuleLoadsForIsolate: only
the in-flight async graph loads still need the explicit early dead-flag +
context-Reset step, because pending NSURLSession completions can hold a
load's shared_ptr past teardown.

Suite: 1208 specs, 0 failures.
…ntries

CompileJsonAsEsModule ran before the resolver's registry-hit check, so every
resolve of the same .json re-read, recompiled, and re-evaluated the file —
minting a new module identity (and namespace) each time. Probe the registry
first; JSON modules are eagerly evaluated, so a registered entry is complete.

LoadESModule short-circuited only kEvaluated entries and recompiled anything
else, replacing a kUninstantiated/kInstantiated module while importers still
held the old one. Reuse those and let InstantiateModule no-op or link.

Suite: 1209 specs, 0 failures.
Both the resolver's referrer discovery and import.meta initialization
identified a module by linearly scanning the whole registry and comparing
handles — O(n) per import, O(n^2) over a module graph. The loader state now
keeps an identity-hash -> registry-keys index (v8::Module::GetIdentityHash,
the module_wrap.cc approach), maintained at every registry insert and in
RemoveModuleFromRegistry; lookups verify candidates against the registry and
prune stale ones, so an overwritten slot can never satisfy a lookup.

The hardcoded runtime.mjs fallback referrer is gone: an unregistered referrer
resolves relative specifiers against the application root, loudly. Every
registered module is in the index, so that path now only means the referrer
truly was never registered. RemoveModuleFromRegistry takes its isolate
explicitly instead of trusting Isolate::GetCurrent.

Suite: 1210 specs, 0 failures.
The resolver previously answered a disk specifier by calling
ModuleInternal::LoadScript, which compiled, instantiated, AND evaluated the
dependency inside the importer's own instantiation — so disk graphs ran in
resolver order instead of the spec's evaluation order, recursively. The
~300-line machinery that existed to survive that (ResolutionStackGuard,
re-entry counters, primary-importer ownership, NS_HMR_MODULE_IN_FLIGHT
gating, fallback-module substitution, the module waiter queue, the _ns_hmr
Documents mirror, the underscore path heuristic) is deleted with it.

The resolver now compiles + registers only (ModuleInternal::CompileFileEsModule,
shared with LoadESModule's own compile step) and returns; V8 drives the graph
walk by resolving the new module's requests during the importer's
InstantiateModule, and cycles terminate through the registry the same way
Node/Blink break them with the module-map self-insert. Evaluation happens
once, at the root. Dynamic-import coalescing of an already-evaluating module
now leans on V8 directly: Evaluate() on a kEvaluating module returns its
existing top-level capability promise, so the TLA chain needs no waiter
bookkeeping.

Also renames the per-isolate state aliases to drop the no-longer-true g_
prefix; g_ remains only on the genuinely process-wide import-map/volatile
globals.

Suite: 1212 specs, 0 failures (adds spec-order diamond + cycle coverage).
…adlines

ns:module loses setDevBootComplete. Its only functional consumer was the
atomic gating the cold-boot runloop pump inside synchronous HTTP fetches,
and the runtime knows that window first-hand: a thread-local depth counter,
armed RAII-style by ModuleInternal::RunModule, now limits the pump to
exactly the entry-evaluation window on the fetching thread. A worker
booting can no longer arm the main thread's pump, and there is no client
signal to forget. The __NS_HMR_BOOT_COMPLETE__ global goes with it; the
surface is unreleased and clients feature-detect members.

The module-graph waits collapse onto one constant
(kModuleEvaluateDeadlineSeconds): the HTTP TLA settle window and the pumped
graph walk use it directly, the NativeScript.mm boot handoff is explicitly
its double as the outermost backstop, so the waits stay ordered by
construction (transport < settle < backstop). Local modules keep a short
settle window deliberately — only nestable V8 tasks can run in-pump, so a
TLA parked on a foreground task can never settle there; the loop yields
after 1s and the real event loop finishes the TLA after the turn
(EventLoopTests pins this). The settle loop also checks promise state
before its first pump, so synchronous graphs exit without a runloop slice.

Suite: 1212 specs, 0 failures.
Async module-graph fetch completions previously hopped to a CFRunLoop
captured at load start (GetCurrentRuntime()?RuntimeLoop():CFRunLoopGetCurrent).
For an import() issued from a GCD background thread that guess captured the
bg thread's never-run runloop, so completions were posted to a dead loop and
the import hung. Completions now post as nestable v8 foreground tasks via
NativeScriptPlatform::LookupEventLoop — delivery is a property of the
isolate, not the calling thread; a null lookup (disposed isolate) drops the
post; the jsLoop capture/retain plumbing is gone. The pumped graph walk
drains nestable tasks directly, keeping its short RunInMode slice only as
the idle-wait.

Teardown ordering: a post the shut-down loop rejects is destroyed on the
POSTING (background) thread, so ~Runtime now quiesces the loads (dead flag +
context-Global Reset) BEFORE EventLoop::Shutdown — a late-dropped task holds
only inert state.

Tests: dynamic imports from a dispatch_async background queue (the local
variant is the regression guard; the HTTP variant is quarantined under the
known Embassy limitation — the in-runner server answers no module GET from
the app, all four transport attempts included, see QUARANTINED_TESTS.md #3).
Graph fixtures self-initialize their order array and the bg spec uses its
own fixture so module single-evaluation cannot couple specs; new specs
report rejections via non-throwing expects (this Jasmine's fail() throws,
which turned assertion failures into opaque timeouts).

Suite: 1214 specs, 0 failures.
The in-runner Embassy server (vendored selector event loop, ~2,900 lines)
never answered the app's module GETs — accepted sockets were half-dead by
the time it serviced them (getPeerName EINVAL), which is why the URL Key
Canonicalization specs were quarantined and the background-thread HTTP
import spec joined them. Prior fixes made the server not-crash; this makes
it serve: ModuleTestServer is a ~200-line Network.framework NWListener on
IPv4 loopback with the same handler contract (environ / startResponse /
sendBody / swsgi.input-shaped body reader), one request per connection,
Connection: close, and thread-safe single-shot response closures so the
delayed timeout.mjs route can answer from another queue.

QUARANTINED_SPEC_SUBSTRINGS is now empty: the canonicalization specs and
the background-thread HTTP import pass against the new server (6ms instead
of 20s of transport timeouts), and the worker-teardown stress spec passes
too — its AB-BA cross-isolate Locker deadlock was fixed by #428 (issue
#420), already on main. QUARANTINED_TESTS.md keeps the mechanism and the
resolved history.

Suite: 1214 specs, 0 failures, all executing.
The worker message queue previously stayed buffered until the entry script
installed an onmessage HANDLER, enforced by a 50ms dispatch_after polling
retry (drainRetryPending_ + SignalMessageDrain) — a stronger-than-web
contract that also checked only the onmessage property, so addEventListener
users would have buffered forever.

HTML's contract is different: the implicit port's message queue is enabled
once the worker script finishes evaluating (including waiting out top-level
await), and from then on messages dispatch whether or not a listener exists
— a handler registered later misses earlier messages. Implement exactly
that: Worker.mm arms WorkerWrapper::EnableMessageQueue after RunModule for
settled entries, and chains it on the entry's evaluation promise when a
local top-level await outlived the settle window
(ModuleInternal::PendingEntryEvaluation probes the promise via re-Evaluate —
a TLA-parked module reports kEvaluated while its capability promise is still
pending, so the status enum cannot detect this). The polling retry and the
handler-presence check are deleted.

New spec pins the web behavior: a message posted before evaluation is
dispatched (and dropped) before a timer-registered onmessage exists; only
messages posted afterwards arrive. Worker realms carry only the native
__ns__-prefixed timers, hence the fixture's __ns__setTimeout.

Suite: 1215 specs, 0 failures.
Debug builds swallowed module-loading failures at fourteen sites — returning
success from RunModule ("telling iOS it succeeded"), empty values from
LoadImpl/LoadScript/LoadESModule/LoadClassicScript, and a dummy require that
silently returned undefined — while release threw. Two sites were inverted:
LoadHttpModuleForUrl and CompileJsonAsEsModule threw only in debug and
silently returned empty in release. A debug build that hides failures is
strictly worse than one that reports them; the dev overlay and the error
modal are presentation, not recovery.

One contract now: failures propagate identically in both build types; debug
keeps its extra logging (LogError, the rejection modal, the classification
diagnostics) and then fails the same way release does. The worker rethrow
that routes entry errors to worker.onerror is preserved. The non-HTTP
top-level-await yield stays non-throwing — that is hand-off-to-event-loop
semantics, not a swallowed failure.

RunMainScript now consumes RunModule's result and throws on failure in every
build: an app whose main module did not run has no defined state to continue
in.

Suite: 1215 specs, 0 failures.
…essage

The bool + outErrorMessage contract was a vestige of the deleted
debug-swallows: every failure site inside already throws
NativeScriptException, RunModule caught them only to flatten into
bool+string, and RunMainScript immediately re-inflated the string into a new
exception. NativeScriptException is the designed carrier for this boundary —
it captures the live V8 error when one exists (the TryCatch constructor) and
converts back to a JS error wherever JS is the audience — so let it travel.

RunModule (ModuleInternal + the Runtime forwarder) is now void and throws;
SetOutErrorMessage and the out-param are gone. Boundaries translate for
their audience: RunMainScript logs the cause and rethrows (a failed main
module is fatal in every build); Worker.mm re-arms the exception on the
isolate via ReThrowToV8 so the original JS error reaches worker.onerror
through the existing TryCatch pipeline unchanged; the inspector logs and
continues (optional tooling, visible but non-fatal). The sync/strict/async
RunModule variants stay future work — they should be born with createRequire
and their real consumers rather than as dead code here.

Suite: 1215 specs, 0 failures (worker onerror specs pin the round-trip).
…es async graphs

One primitive, EvaluateModuleGraph, now owns graph evaluation under three
named policies:

- kSyncStrict: Node's require(esm) semantics. IsGraphAsync() is refused
  before evaluation ever starts (the module stays instantiated and
  loadable via import()), including on registry hits already at
  kEvaluated - a TLA-parked graph reports kEvaluated with a pending
  capability promise, so returning its namespace would hand require() a
  TDZ binding. Sync graphs must settle synchronously; the promise state
  is read directly, no pumping.
- kSyncPumping: the boot/worker-entry settle loop, unchanged: nestable
  v8 tasks + microtask checkpoints, HTTP-only runloop slices, 1s local
  yield window (return-pending) vs 60s HTTP deadline (throw).
- kAsync: evaluate and hand back the capability promise (the dynamic
  import shape; the import() machinery is unchanged for now).

require() of an .mjs previously fell into the pumping path by accident
via LoadScript -> LoadESModule; it is now kSyncStrict, matching Node's
ERR_REQUIRE_ASYNC_MODULE behavior. The EventLoopTests TLA spec asserts
the refusal and that import() of the refused module still works, before
and after evaluation.
require() of an ES module now hands back what Node's
populateCJSExportsFromESM produces instead of the raw namespace:

- an export literally named 'module.exports' wins outright (it can be a
  function or a primitive, hence exports is now a Value);
- a namespace with no default export, or one declaring its own
  __esModule, passes through unchanged;
- otherwise a synthetic facade module re-exports the target and adds
  __esModule = true, so transpiled consumers reading
  `_mod.__esModule ? _mod.default : _mod` find the real default.
  Re-exports keep live bindings and enumerability, which a copied
  object would not.

The facade matches Node byte-for-byte (source and the dedicated
resolve-callback linking shape of CreateRequiredModuleFacade). One
facade per target module, cached by target identity hash with handle
comparison, dropped when the target leaves the registry so eviction can
never leave a facade over a dead module. EvaluateModuleGraph moves to
the shared header so the facade evaluates under the same kSyncStrict
policy.

import() continues to observe the raw namespace, before and after a
require() of the same module - also Node's behavior.
…ode:module shim

ns:module grows to five members. createRequire(filenameOrURL) mints a
require bound to the given base with Node's argument contract (absolute
path, file: URL string, or URL object; TypeError otherwise; http(s)
bases refused since require() of dev-served modules stays blocked) and
Node's require(esm) semantics: async graphs are refused. The new
node:module builtin re-exports createRequire only, keeping the node:
surface Node-shaped; no .d.ts is shipped for it because @types/node
already declares that ambient module.

createPumpingRequire is the NativeScript-specific escape hatch for
native-boundary calls: an async graph is driven to settlement with
nestable v8 tasks and microtask checkpoints under the 60s deadline,
then throws rather than returning a half-initialized namespace. It
never advances the Cocoa runloop - outside boot the runloop belongs to
the app, and re-entering arbitrary runloop sources from a require is
the hazard the boot pump gets disarmed against.

The policy travels explicitly through the require plumbing
(require-factory closure -> RequireCallback info[2] ->
LoadImpl/LoadModule/LoadScript -> LoadESModule), strict by default, so
a module's nested requires inherit how it was loaded. LoadESModule now
takes full ModuleEvaluationOptions, with named builders separating boot
entries (local 1s yield window) from explicit pumping requires.

The async-graph refusal now names createPumpingRequire. Also fixes the
tilde specifier: require("~foo") resolved through substr(2) and lost
its first letter; ~/foo and ~foo now both resolve against the app root.

The builtin JS keeps the primordials discipline: file: URL parsing is
string-based (no replaceable URL global) and decodeURIComponent is
snapshotted.
Replaces the ad-hoc trace patchwork (bracket tags invented per call
site, gates hand-rolled per site, everything on OS_LOG_DEFAULT) with one
facility shaped like Node's debug_utils.h:

- LogCategory {esm, fetch, registry} indexing one relaxed-atomic
  bitmask - a plain load on arm64, no strings or maps on the hot path.
- TNS_DEBUG is a macro, not a function: argument evaluation itself sits
  inside the [[unlikely]] cold branch, so disabled call sites never pay
  for the .c_str() expressions they pass.
- EmitDebugLog lives out of line and writes through a cached
  per-category os_log_t under subsystem org.nativescript.runtime, so
  Console.app / log stream filter by category natively. The printf
  format attribute also surfaced a pre-existing NSString*-into-%s crash
  bug, fixed here.
- Enabled in release builds too: these are trace logs, and a build that
  cannot be traced cannot be diagnosed. Failure behavior is unchanged.

Enablement: the NS_DEBUG environment variable at startup (the only way
to trace boot), and setConfig('debug', 'esm,fetch') on ns:runtime at
runtime - each write replaces the whole set. The logScriptLoading and
httpFetchUrlLog config keys are gone; the category name supplies the
log prefix, so the [ns-hmr] tag dies (those sites were registry
diagnostics, not HMR - now under registry).
…otask

The pump advances the loop with nestable tasks and microtask
checkpoints, and V8 ignores a checkpoint while the isolate is already
draining the microtask queue. A top-level await resumes through a
promise reaction, so createPumpingRequire called from a microtask turn
(after an await, inside a .then) could never settle such a graph - it
burned the full 60s deadline and then threw.

Refuse it up front instead, before evaluation, so the graph stays
instantiated and import() can still load it. The guard is compound:
only kSyncPumping + IsGraphAsync + IsRunningMicrotasks trips it - a
synchronous graph needs no pumping and stays legal from any context,
and boot/worker entries arrive from native at task level.

The specs also stop trusting their own call context: Jasmine continues
its queue synchronously out of the previous async spec's done(), so a
spec that needs a clean task hops there via __ns__setTimeout first.
The runtime provides mechanism; the client provides vocabulary. Five
places string-matched @nativescript/vite (and in two cases Angular/Vue)
conventions instead:

- the '@' sentinel complex: import('@') fabricated and registered an
  empty stub module, the resolver and graph walk swallowed the
  specifier, and eviction refused to touch the fake key. A bare '@' now
  fails loudly as the unresolvable specifier it is.
- diagnostic labels keyed on /@ns/sfc/, /@ns/m/, and .component URL
  shapes.
- NormalizeViteSpecifier: the .vite/deps esbuild-flattening reversal
  and /node_modules package extraction that gave the import map a
  second-chance lookup. The plugin knows its own rewrites and emits
  exact import-map keys instead.
- the unconfigured canonicalization fallback (built-in dev-endpoint
  prefixes, /@ng/component, t/v/import strip-params). Unconfigured
  canonicalization is now purely mechanical - fragment strip only -
  because which params are cache-busters is knowledge only the client
  has, and guessing collapses distinct modules onto one registry key.
  The configured path is unchanged.
- compile-error classification arms matching Vue compiler output
  (__sfc__, openBlock).

Also removes the dead 'optional:' scheme arms left from the deleted
optional-module placeholder.

Each removal's client-side replacement is recorded for the plugin
authors (configureLoader vocabulary, exact import-map keys, or a
client fix); canonicalization specs now install their vocabulary
explicitly instead of leaning on the deleted fallback.
… parse

CompileModuleForResolveRegisterOnly caught the compile exception to
build a debug-only diagnostic, then returned an empty handle without
rethrowing - the SyntaxError that named the module, line and column was
destroyed in both builds, and the importer saw a generic resolve or
'HTTP import compile failed' error with the cause gone. In release
there was no debug log either, so the real error was unobservable
anywhere.

The exception is now left pending (the same contract as
CompileFileEsModule), and each caller handles it for its context: the
resolver paths propagate it; the async graph walk and the blob import
callback consume it into their own failure channels (the load's
failureMessage / the waiter rejection), where a pending exception would
otherwise leak into the microtask checkpoint or V8's promise machinery.

The 75-line IsDebug diagnostic block - hash, snippet, classification
heuristics - existed to compensate for the swallow and is replaced by
one category-gated trace line. The registry-reuse lookup also moves
above the compile, so a hit no longer pays for compilation.

A served /esm/syntax-error.mjs fixture pins that the rejection now
carries both the module URL and the real parse error.
Two stragglers migrate to TNS_DEBUG (the no-referrer fallback narration
and the worker JSON-module wrap, which also loses its arbitrary
isolate-kind gate along with the two resolver trace lines that only
fired for workers). The rest of the raw Log() population - 41 sites -
is failure observability (FATAL lines, exception dumps, warn-and-throw
companions) and stays plain Log by design.

Deleted outright: the no-op self.onmessage probe in LoadModule (read
two properties, used neither), the worker JSON content-preview and
resolve-failure printfs (the throw two lines later carries the same
facts), and Worker.mm's five commented-out printf blocks, one of which
kept a live exception-string extraction alive as its only consumer.
CompileJsonAsEsModule loses its now-unused isWorker parameter.
The legacy polyfill was a C++-embedded source string compiled inside
ResolveModuleCallback: import-only (require("node:url") failed), a
registry-resident module rather than a per-realm frozen singleton, no
primordials discipline, and wrong on the details (a file://host/ URL
kept the host in the path, non-file strings passed through unchanged,
query and fragment leaked into the result).

node:url is now a registered builtin like node:util and node:module -
lazy, frozen, reachable through require and import with one identity.
Parsing goes through the URL intrinsic (snapshotted in primordials;
installed on the global template, so always present at capture time):
authority folding, percent-decoding and canonicalization follow the
spec. Node-strict where the polyfill was lax: non-file schemes throw,
a real remote host throws, %2F in the path throws rather than decoding
a separator, and pathToFileURL requires an absolute path - there is no
working directory to resolve against, so any answer would be invented.
The surface stays deliberately partial: the two converters only.

Also aligns both resolver entry points from IsRegistered||IsNsScheme
to IsBuiltinScheme, so an unregistered node: specifier now fails with
"No such built-in module" uniformly instead of falling through to
filesystem resolution on the import path.
BuildNsModuleBinding lived in HttpLoader.mm only because that file
used to be the whole dev surface. The registry convention is that a
builtin's binding is assembled by the subsystem that owns the
functionality, and ns:module's functionality is the module loader - so
the binding, its InstallDevFunction helper, the three callbacks, and
the surface comment move to ModuleInternalCallbacks. HTTP specifics
stay in HttpLoader behind exported functions; CanonicalizationConfig
and SetCanonicalizationConfig are promoted to its header as the one
dependency the moved code needs. HttpLoader.h also drops its v8
forward-declaration block, which the binding was the last user of.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants