Skip to content

module: fix wrong error annotation for require of ESM#62685

Open
om-ghante wants to merge 1 commit intonodejs:mainfrom
om-ghante:fix/55350-wrong-error-annotation-require-esm
Open

module: fix wrong error annotation for require of ESM#62685
om-ghante wants to merge 1 commit intonodejs:mainfrom
om-ghante:fix/55350-wrong-error-annotation-require-esm

Conversation

@om-ghante
Copy link
Copy Markdown

When a CommonJS module requires an ES module with
--no-experimental-require-module, the error annotation (arrow message)
points to an internal frame instead of the user's actual require() call.

Before (broken)

The error annotation incorrectly points to node:diagnostics_channel:315
with content undefined, because TracingChannel.traceSync is the first
frame in the stack trace.

After (fixed)

The error annotation correctly points to the user's file and line where
require() was called, showing the actual source line.

Root Cause

The reconstructErrorStack function always picked the first at frame
from the error stack trace. Since v22.4.0, require() is wrapped by
TracingChannel.traceSync via wrapModuleLoad, so the first frame is
the internal tracing wrapper instead of the user's code.

Fix

Search the stack frames for one matching the parent module's file path,
instead of blindly using the first frame. This also addresses an
existing TODO comment about trimming internal frames.

Fixes: #55350

When a CommonJS module requires an ES module with
--no-experimental-require-module, the error annotation (arrow message)
was pointing to an internal frame (TracingChannel.traceSync in
node:diagnostics_channel) instead of the user's actual require() call.

This happened because reconstructErrorStack() was always picking the
first 'at' frame from the error stack trace. When the require() call
is wrapped by TracingChannel.traceSync (added in v22.4.0 via
wrapModuleLoad), the first frame is the internal tracing wrapper, not
the user's code.

Fix reconstructErrorStack() to search the stack frames for one that
matches the parent module's file path, instead of blindly using the
first frame. This ensures the error annotation correctly points to the
user's require() call.

Also remove a TODO comment that this change addresses.

Fixes: nodejs#55350
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Apr 10, 2026
@afurm
Copy link
Copy Markdown

afurm commented Apr 11, 2026

Nice improvement to the error annotation. The loop to find the matching stack frame is more robust than grabbing the first frame.

One concern: err may be mutated. Between getRequireESMError throwing and reconstructErrorStack running, other code could potentially mutate err.stack (especially in userland or with async context). The loop assumes err.stack is stable by the time it runs. Is there any scenario where the stack could be reformatted or truncated before this executes? The early return when errLine === undefined handles the missing frame case safely.

Also: The TODO comment // TODO(joyeecheung): trim off internal frames from the stack is removed but the issue is not fully solved by this change — internal frames from TracingChannel.traceSync are now skipped, but other internal wrappers could still appear before the user's frame. Is there a more general approach being considered, or is this fix scoped to the reported issue? Not blocking, just curious about the longer-term direction.

@om-ghante
Copy link
Copy Markdown
Author

Nice improvement to the error annotation. The loop to find the matching stack frame is more robust than grabbing the first frame.

One concern: err may be mutated. Between getRequireESMError throwing and reconstructErrorStack running, other code could potentially mutate err.stack (especially in userland or with async context). The loop assumes err.stack is stable by the time it runs. Is there any scenario where the stack could be reformatted or truncated before this executes? The early return when errLine === undefined handles the missing frame case safely.

Also: The TODO comment // TODO(joyeecheung): trim off internal frames from the stack is removed but the issue is not fully solved by this change — internal frames from TracingChannel.traceSync are now skipped, but other internal wrappers could still appear before the user's frame. Is there a more general approach being considered, or is this fix scoped to the reported issue? Not blocking, just curious about the longer-term direction.

Thanks for the review and the thoughtful questions!

Regarding your first concern (mutation of err.stack):
The stack trace should be entirely stable here. getRequireESMError is thrown, immediately caught within loadSource (or its caller), and then reconstructErrorStack is called synchronously right after. There is no asynchronous boundary or tick cycle where userland code could intercept or mutate the stack before reconstructErrorStack executes. Even in edge cases where a process-level uncaughtException handler intercepts it, the if (errLine === undefined) return; acts as a solid fail-safe—if the stack is somehow truncated or reformatted, we just gracefully abandon the arrow annotation without throwing a secondary error.

Regarding the TODO and the scope of the fix:
Searching for the frame that matches parentPath is actually intended as the more durable, general-purpose solution compared to manually "trimming" internal frames. If we tried to trim internals by specifically ignoring node:diagnostics_channel or node:internal/modules/..., we run the risk of that list falling out of sync as the internal loader architecture evolves. By dynamically scanning for the first frame that belongs to parentPath (the requiring user module), we are effectively skipping all internal execution wrappers and landing exactly on the caller's call site. In a sense, this achieves the goal of the TODO without needing to hardcode exclusions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong error annotation when commonjs requires an ES module

3 participants