fix: report the rejection reason when ES module evaluation fails - #410
Conversation
With top-level-await semantics, Module::Evaluate() never throws
evaluation errors synchronously - it returns a promise that rejects, so
the TryCatch around evaluation stays empty and the kRejected branch
previously threw a synthetic message-only NativeScriptException
("Module evaluation promise rejected"). The actual error (e.g. a
ReferenceError thrown while evaluating the module graph) was fetched via
promise->Result() but never attached to the exception: the
isolate->ThrowException(reason) call was swallowed by the still-active
TryCatch on scope exit and nothing ever read it. Every outer layer
(instantiate, require) then re-wrapped the placeholder, so users saw
'Module evaluation promise rejected' with no message or stack for the
real failure.
Re-throw the rejection reason on the isolate so the TryCatch captures
it, then construct the NativeScriptException from the TryCatch - the
same treatment synchronous evaluation errors get - so the original
error message and JS stack trace propagate. Also log the error in debug
builds, mirroring the synchronous evaluation failure path.
📝 WalkthroughWalkthrough
ChangesES module error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
NativeScript/runtime/ModuleInternal.mm (1)
888-904: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for rejected top-level-await errors.
Cover an ESM fixture that rejects with an
Error, asserting that both direct loading and therequire()wrapper preserve the original message and stack. The reported Debug build validates compilation, but not this runtime propagation contract.🤖 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 888 - 904, Add a regression test for the top-level-await rejection path around promiseTc and NativeScriptException, using an ESM fixture that rejects with an Error. Assert that both direct module loading and the require() wrapper preserve the original error message and stack, covering the runtime propagation contract in Debug builds.
🤖 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.
Nitpick comments:
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 888-904: Add a regression test for the top-level-await rejection
path around promiseTc and NativeScriptException, using an ESM fixture that
rejects with an Error. Assert that both direct module loading and the require()
wrapper preserve the original error message and stack, covering the runtime
propagation contract in Debug builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59ad066d-e15f-45aa-afb9-c702acfd4ba2
📒 Files selected for processing (1)
NativeScript/runtime/ModuleInternal.mm
What is the current behavior?
When an ES module graph fails during evaluation (e.g. a
ReferenceErrorthrown at module scope), the app crashes with an opaque, layered error that never mentions the actual failure:The real error message and JS stack trace are nowhere in the output.
Root cause: with top-level-await semantics,
Module::Evaluate()never throws evaluation errors synchronously — it returns a promise that rejects. ThekRejectedbranch inLoadESModulefetched the real error viapromise->Result()but never attached it to the thrown exception:The message-only
NativeScriptExceptionconstructor fabricates a newErrorfrom the fixed string, so every outer layer (instantiate of the importer,require()wrapper) re-wraps a placeholder while the original error is lost.What is the new behavior?
The rejection reason is re-thrown on the isolate before constructing the exception, so the still-active
TryCatchcaptures it and theNativeScriptException(isolate, TryCatch&, message)constructor extracts the original error message and JS stack trace — the same treatment synchronous evaluation errors already get. Debug builds also log the error, mirroring the synchronous evaluation-failure path. The message-only fallback is kept in case the TryCatch has nothing (defensive; should not happen).Example: a webpack ESM bundle whose evaluation throws
ReferenceError: NativeClass is not definednow reports that message and the frame in the module graph where it was thrown, instead of the generic placeholder above.Verified with
xcodebuild -scheme NativeScript -configuration Debug -sdk iphonesimulator build(BUILD SUCCEEDED).Summary by CodeRabbit