fix stack trace filename format for transpiled files#398
fix stack trace filename format for transpiled files#398kirkwaiblinger wants to merge 14 commits into
Conversation
|
Hi dear @kirkwaiblinger Do you think you can minimize changes (changes to Also, I noticed typescript-eslint/typescript-eslint#11546 is already merged, fixing the original typescript-eslint issue. Do you think it is still worth upstreaming it? |
👋
It is, actually! This is what decides between a
What would be the value in making this configurable? The most (and only?) important consideration I could think of in deciding how to present the filename would be alignment with node, which uses backslashes on CJS paths (and in particular, a path such that
Yes. |
kirkwaiblinger
changed the title
|
@pi0 I've updated the tests. If you try out various combinations of reverting the changes to eval.ts and running the tests with windows/non-windows, it should hopefully demonstrate pretty clearly the things I'm trying to point out! 🙂 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdated module-type detection and transpile decision logic; resolve module filenames to absolute paths and use file:// URLs for ESM when invoking vm.runInThisContext; set Module.filename to the resolved path and compute __dirname via nodeDirname; added fixtures and a helper to validate stack-trace path formats across CJS/CTS/ESM/MTS modules. ChangesModule type detection, filename formatting, transpile gating, and test fixtures
Sequence Diagram(s)sequenceDiagram
participant Loader
participant FS as Filesystem
participant Analyzer as TypeDetector
participant Formatter as PathFormatter
participant VM
participant ModuleWrapper
Loader->>FS: resolve requested filename (path.resolve)
Loader->>Analyzer: determine isESM / isCommonJS (ext, nearest package.json)
Analyzer-->>Loader: module type
Loader->>Formatter: format filename (file:// URL if ESM, else resolved path)
Loader->>VM: vm.runInThisContext(compiledCode, { filename: formattedFilename })
VM->>ModuleWrapper: instantiate Module with filename = resolvedPath
Loader->>ModuleWrapper: invoke wrapper(..., __dirname = nodeDirname(mod.filename), ...)
ModuleWrapper->>ModuleWrapper: execute module code (CJS or ESM evaluation path)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/fixtures/filename/get-stack-trace.cjs`:
- Around line 3-10: Wrap the temporary modifications of Error.stackTraceLimit
and Error.prepareStackTrace and the call to Error.captureStackTrace(dummyObj,
getTopOfStackTrace) in a try/finally block so that Error.prepareStackTrace and
Error.stackTraceLimit are always restored even if an exception is thrown;
specifically save oldStackTraceLimit and oldPrepareStackTrace before changing
them, perform the capture in the try, and restore oldPrepareStackTrace and
oldStackTraceLimit in the finally block to guarantee global state is reset.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/__snapshots__/fixtures.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/eval.tstest/fixtures/filename/cjs-module.cjstest/fixtures/filename/cts-module.ctstest/fixtures/filename/esm-module.mjstest/fixtures/filename/get-stack-trace.cjstest/fixtures/filename/index.tstest/fixtures/filename/mts-module.mts
| ((ext === ".js" || ext === ".ts") && | ||
| readNearestPackageJSON(filename)?.type === "module"); | ||
| const isCommonJS = ext === ".cjs" || ext === ".cts"; | ||
| const needsTranspile = |
There was a problem hiding this comment.
btw this logic is obviously convoluted. It is equivalent to the following:
const needsTranspile = (() => {
// force takes precedence
if (evalOptions.forceTranspile != null) {
return evalOptions.forceTranspile;
}
// TS always needs transpile, regardless whether ESM or CJS
if (isTypescript) {
return true;
}
// CommonJS skips transpile
if (isCommonJS) {
return false;
}
// In async mode, we can skip transpiling native ESM as well
if (isESM && evalOptions.async) {
return false;
}
return isESM || ctx.isTransformRe.test(filename) || hasESMSyntax(source);
})();which I'd actually prefer to transcribe as
const needsTranspile = (() => {
// force takes precedence
if (evalOptions.forceTranspile != null) {
return evalOptions.forceTranspile;
}
// TS always needs transpile, regardless whether ESM or CJS
if (isTypescript) {
return true;
}
// CommonJS skips transpile
if (isCommonJS) {
return false;
}
if (isESM) {
// In async mode, we can skip transpiling native ESM as well
// otherwise it needs to be transformed
return evalOptions.async;
}
// I don't know what this does
if (ctx.isTransformRe.test(filename)) {
return true;
}
if (hasESMSyntax(source)) {
// Neither explicitly specified as ESM nor CJS, but contains
// ESM syntax. This should be transpiled.
// See https://nodejs.org/api/packages.html#syntax-detection
return true;
}
return false;
})();The only reason I didn't do one of these is that the prettier-ignore made it seem like a previous author wanted the code to presented exactly as it is rather than in a more explicit manner.
The important logic change here is that isTypescript is no longer disjoint from isESM and isCJS. Distinguishing isTypescript && isESM from isTypescript && isCJS is necessary to present stack traces correctly for transpiled TS. Thus, now that isTypescript && isCommonJS is possible, the isTypescript check needed to be moved before the isCommonJS, else the isCommonJS check would cause needsTranspile to short-circuit to false.
Use mlly's `pathToFileURL` (returns a string) instead of the `node:url` one, and inline the IIFE.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/eval.ts`:
- Around line 148-154: mod.filename and the injected __filename are inconsistent
on Windows because resolvePath(filename) is computed only for
vm.runInThisContext (formattedFileName) but mod.filename is left as the original
pathe-normalized filename; to fix, compute resolved = resolvePath(filename) once
early (reuse the same value) and set mod.filename and the value passed into the
module wrapper's __filename to the OS-native resolved/formatted variant (i.e.,
reuse formattedFileName for vm.runInThisContext and ensure __filename uses the
same resolved path for CommonJS); update usages in resolvePath,
formattedFileName, mod.filename, and the module wrapper invocation so __filename
and stack-trace filenames match on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Looks like the test failure is due to bun formatting the filenames in stack traces differently (path rather than file URL) for ESM. Filed oven-sh/bun#30298. Regardless of whether the bun issue is accepted and/or fixed, I guess it wouldn't hurt to update the code/tests to be more lenient around whether a url vs a path is detected? Though one might need to have separate snapshot outputs for node vs bun vs deno then. Please advise 🙏 |
2 participants
resolves #397
Summary by CodeRabbit
New Features
Tests