lib: cache source maps in vm sources #52153
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
|
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 @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 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. |
| @@ -151,6 +152,7 @@ function internalCompileFunction( | |||
| } | |||
|
|
|||
| registerImportModuleDynamically(result.function, importModuleDynamically); | |||
| maybeCacheSourceMap(filename, code, result.function, false, result.sourceURL, result.sourceMapURL); | |||
There was a problem hiding this comment.
What’s the difference between a “source URL” and a “source map URL”? My naïve guess is that a source URL is a file: URL to the module itself, and a source map URL is either a file: URL to a .map file or a data: URL with the source map. I feel like these guesses are probably wrong, though, so these variables should at least get comments or perhaps get better names (since you reuse these names across many files).
There was a problem hiding this comment.
Your guess is correct. SourceURL is used to specify the URL of the eval-ed source, as described in https://tc39.es/source-map/#linking-evald-code-to-named-generated-code. However, it can be present in sources loaded by CJS/ESM loaders from filesystem as well. V8 will take this magic comment and report errors with it as the script resource name instead:
Lines 62 to 65 in f1949ac
The name, source url, is taken from the magic comments as:
//# sourceMappingURL=<url>
//# sourceURL=foo.js
| // To reproduce: | ||
| // cd test/fixtures/source-map | ||
| // npx --package=coffeescript@2.5.1 -- coffee -M --compile tabs.coffee | ||
| // sed -i -e "s/$(pwd | sed -e "s/\//\\\\\//g")/\\/synthethized\\/workspace/g" tabs.js |
github-actions
bot
removed
the
request-ci
|
Yeah, I can see the potential memory management advantages if v8 translates the source maps. For instance, the script object of |
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
legendecas
force-pushed
the
source-maps/vm
branch
from
23bdaed to
44aa272
Compare
|
Updated to fix Windows CI failures about the drive letters. |
|
This needs a rebase. |
|
What are the next steps to get this merged? This is affecting sourcemaps in bundlers like Webpack and Turbopack that use |
|
I'm going to rebase to the latest main branch... |
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.
legendecas
force-pushed
the
source-maps/vm
branch
from
44aa272 to
ccc60bf
Compare
Codecov ReportAttention: Patch coverage is
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
|
github-actions
bot
removed
the
request-ci
|
Should this have affected 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? |
@eps1lon https://github.com/eps1lon/vm-sourcemaps specified an external realtive source map url, but |
github-actions
bot
removed
the
request-ci
Cache source maps found in sources parsed with
vm.Script,vm.compileFunction, andvm.SourceTextModule. Also, retrieve sourceurl from V8 parsing results.
Not like filenames returned by
CallSite.getFileName()in translatingstack 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: #52102