node
node copied to clipboard
nodejs
lib: cache source maps in vm sources
Cache source maps found in sources parsed with vm.Script,
vm.compileFunction, and vm.SourceTextModule. Also, retrieve source
url from V8 parsing results.
Not like filenames returned by CallSite.getFileName() in translating
stack traces, when generating source lines prepended to exceptions,
only resource names can be used as an index to find source maps, which
can be source url magic comments instead. Source url magic comments
can be either a file path or a URL. To verify that source urls with
absolute file paths in the source lines are correctly translated,
snapshots should include the full snapshot urls rather than
neutralizing all the path strings in the stack traces.
Fixes: https://github.com/nodejs/node/issues/52102
Can this let us have V8 produce correct stack traces in the first place so that we don't need to rewrite them?
V8 doesn't parse JS source maps because it is not necessary to run the code. Source maps are debugging tools that impose runtime performance costs when enabled. In Chrome, only stack traces shown in Chrome DevTools are translated. With the development flag --enable-source-maps, stack traces are translated in Node.js as well.
@GeoffreyBooth are you suggesting that to have V8 translating the stack trace with source maps instead?
@GeoffreyBooth are you suggesting that to have V8 translating the stack trace with source maps instead?
Yeah. If possible, it would be nice to have V8 handle some or all of the work that we do in lib/internal/source_map/* and the source map-related logic in node_errors.cc. A lot of it is stack trace rewriting per the source map. I don’t know if that’s something that V8 supports as part of its general source map support, but if it is, it would be nice to use that rather than implementing it ourselves.
Not something that needs to be part of this PR, just something that felt like an easy win if you saw the potential for it. Besides reducing our maintenance burden, I’d imagine that V8 source map support would probably be faster than ours.
Yeah, I can see the potential memory management advantages if v8 translates the source maps. For instance, the script object of eval-ed sources was not exposed through embedder API so it is permanently cached on our side. It is a really good point that if the source map implementation could be moved to V8 and I found that V8 also implemented Wasm source map support.
What are the next steps to get this merged? This is affecting sourcemaps in bundlers like Webpack and Turbopack that use vm.runInThisContext instead of (0, eval).
Codecov Report
Attention: Patch coverage is 74.35897% with 10 lines in your changes missing coverage. Please review.
Project coverage is 88.00%. Comparing base (
3bb1d28) to head (ccc60bf). Report is 1589 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #52153 +/- ##
==========================================
+ Coverage 87.95% 88.00% +0.04%
==========================================
Files 656 656
Lines 188310 189005 +695
Branches 35963 35994 +31
==========================================
+ Hits 165630 166331 +701
+ Misses 15854 15837 -17
- Partials 6826 6837 +11
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/modules/esm/translators.js | 92.88% <100.00%> (ø) |
|
| lib/internal/modules/esm/utils.js | 98.88% <100.00%> (ø) |
|
| lib/internal/vm.js | 100.00% <100.00%> (ø) |
|
| lib/internal/vm/module.js | 98.47% <100.00%> (+<0.01%) |
:arrow_up: |
| lib/vm.js | 99.28% <100.00%> (+<0.01%) |
:arrow_up: |
| lib/internal/modules/cjs/loader.js | 94.30% <66.66%> (ø) |
|
| src/module_wrap.cc | 75.76% <60.00%> (-0.11%) |
:arrow_down: |
| src/node_errors.cc | 64.37% <62.50%> (-0.19%) |
:arrow_down: |
| src/node_contextify.cc | 81.33% <71.42%> (-0.04%) |
:arrow_down: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Should this have affected vm.runInContext? I can't get vm.runInContext to sourcemap stack traces when I use Node.js built from this branch. Full repro: https://github.com/eps1lon/vm-sourcemaps.
Though tests seem to indicate otherwise. I'd like to check if this fixes the issues we're seeing in Next.js but maybe I'm missing a special command to use Node.js from this branch? node --version is showing v24.0.0-pre so I thought it does use the locally built Node.js.
Should this have affected
vm.runInContext? I can't getvm.runInContextto sourcemap stack traces when I use Node.js built from this branch. Full repro: https://github.com/eps1lon/vm-sourcemaps.Though tests seem to indicate otherwise. I'd like to check if this fixes the issues we're seeing in Next.js but maybe I'm missing a special command to use Node.js from this branch?
node --versionis showingv24.0.0-preso I thought it does use the locally built Node.js.
@eps1lon https://github.com/eps1lon/vm-sourcemaps specified an external realtive source map url, but vm.runInContext did not specify an absolute URL so vm.runInContext can not resolve the source map. To make such case work, specify an absolute filename in vm.runInContext.
I'd wait until the new granularity control module.setSourceMapsSupport API is available in https://github.com/nodejs/node/pull/56639 before landing this.
Hey @legendecas, any updates here? And any chance of a v20 backport if this lands? 🙏
