lib: fix inspector links for stack traces #35725
Conversation
|
Review requested:
|
github-actions
bot
removed
the
request-ci
Dragiyski
changed the title
Codecov Report
@@ Coverage Diff @@
## master #35725 +/- ##
==========================================
- Coverage 96.40% 96.40% -0.01%
==========================================
Files 222 223 +1
Lines 73682 73713 +31
==========================================
+ Hits 71034 71061 +27
- Misses 2648 2652 +4
Continue to review full report at Codecov.
|
github-actions
bot
removed
the
request-ci
|
This could use some reviews. @nodejs/inspector @nodejs/assert @nodejs/modules-active-members @nodejs/testing @bcoe (source maps) I keep meaning to take a closer look and don't, so I'll ask my question here without having done that (sorry): How does this affect non-inspector output? Will file URLs start showing up in my stack traces at the command line? Or is this inspector-output only? |
Yes, it will affect the console output (stderr/stdout) and the inspector output. Current implementation seems inconsistent, there are already URLs in the output when ESM modules are involved: // error.mjs
export default function test() {
throw new Error();
}// main.js
(function () {
'use strict';
(async () => {
const test = await import('./error.mjs');
test.default();
})().catch(error => {
console.error(error);
});
})();There is also URLs even the error is thrown in the CJS module: // main.mjs
import test from './error.js';
test();// error.js
module.exports = function () {
throw new Error();
};As it can be seen, ESM modules already report local files as URLs. If any module on NPM examine P.S. It might be useful to allow using |
|
It looks like this needs some changes for tests to pass on Windows. Not entirely sure why those are showing up in Jenkins but not in GitHub Actions CI, but I think it's because we have to skip a bunch of |
We don’t currently run any tests on Windows in GitHub actions. We used to in the workflow for testing building from the source tarball until #34440. |
Dragiyski
force-pushed
the
fix-inspector-filename
branch
from
3124565 to
8b5bf16
Compare
|
I'm excited to have another person looking at stack traces 😄 A couple initial questions (I'll give a more thorough review in the next couple days).
I will be curious to hear other people's feedback. The addition of the scheme seems a little redundant for the terminal output, but I appreciate that it makes for a better experience in the inspector. |
Yes, when source maps are enabled, the line plugged in into the stack is clickable and links to the source mapped by the source map. The original line is also shown and it leads to the minified file. Chrome's devtools client have some kind of link detection algorithm. You can put in the console any link (including Anyway, the idea here is to register all scripts in
I tried to address that in |
Dragiyski
force-pushed
the
fix-inspector-filename
branch
from
ab38a76 to
9df7683
Compare
There was a problem hiding this comment.
It seems having slashes (like node://) helps to detect URL in general, but it does not lead to any registered files. I think that's a bug the devtools devs must fix, but they probably won't because, in websites, all call sites that have filename (they are not native C++) are URLs (scripts in HTML page or remote scripts).
It might be worth talking to some folks who work on inspector, see if they have any thoughts. Where would we want node: to link to? the node.js docs?, the source code on GitHub?
tried to address that in sourcesToAbsolute, but there is no safe portable way to manually build a file:// URL using string concatenation and I do not see why we need to do that.
I'm less concerned about the unit tests passing (I'm sure you'll end up finding the edge cases causing problems).
What I'd like to make sure, is that the inspector UI works as we'd hope on a few testing platforms:
- you've already tested that adding schemes makes the user experience better on OSX/Linux ✅ .
- It would be good to have someone run an inspector session on Windows and confirm the same UX improvement.
- I'd be curious to see how VS Code handles this new style of errors, does it link the error paths too?
| const line = call.getLineNumber() - 1; | ||
| let column = call.getColumnNumber() - 1; | ||
| let identifier; | ||
| let code; | ||
|
|
||
| if (filename && StringPrototypeStartsWith(filename, 'file:')) { | ||
| try { | ||
| filename = fileURLToPath(filename); |
There was a problem hiding this comment.
With the check for file:, under what conditions do we expect this to fail?
If we leave this try/catch, we might want to put a debug in the catch.
There was a problem hiding this comment.
fileURLToPath and normalizeReferrerURL call URL constructor with a single argument. There was a string (I do not remember exactly, but I can try to remove the try/catch and rebuild to find it) like [eval something] that makes URL throws an ERR_INVALID_URL error. Since that is in the unit tests, it might be used in the future. Moreover, users of vm can pass any string for filename or have source map containing any string. The combination of both is possible to make otherwise invalid URL to appear as valid module and in general invalid URLs supplied by the user should not crush for the proper reason: MODULE_NOT_FOUND for example.
There was a problem hiding this comment.
Might be worth getting a unit test around the edge case if possible 👌
lib/internal/modules/cjs/loader.js
Outdated
| @@ -995,10 +995,14 @@ function wrapSafe(filename, content, cjsModuleInstance) { | |||
| }); | |||
| } | |||
| let compiled; | |||
| let filenameUrl = filename; | |||
| try { | |||
| filenameUrl = normalizeReferrerURL(filename); | |||
There was a problem hiding this comment.
likewise, if we can instead test for filename patterns that would make normalizeReferrerURL throw, it might be better than a catch all try/catch.
| // If source is any form of full URL (for any protocol) | ||
| // it will remain full URL. If it is relative, it will | ||
| // become file:// url, because the baseUrl is file URL. | ||
| source = (new URL(source, baseUrl)).href; |
There was a problem hiding this comment.
I just ended up undertaking close to this same refactor in #35903
There was a problem hiding this comment.
Yes, that function is affected in both issues. I traced its usage to ensure the first parameter is always a directory path and then converted it to URL. It seems your solution is to first convert it to URL and then call sourcesToAbsolute. Please, make sure it ends with slash /. WHATWG rules for URL will threat the last part as as file otherwise and return one directory upwards. Otherwise both solutions are identical in their core: new URL(source, baseURL).href.
P.S. I am not familiar with the nodejs source, so I try to minimize the amount of dependent changes.
I tested the behavior of devtools on both Windows 10 (Pro June 2020 with Chrome 86.0.4240.111, on VirtualBox) and Ubuntu 20.04, it shows identical behavior:
With both Webstorm (2020.2.3 on Ubuntu 20.04) (Screenshot) and VS Code (configuration above) (Screenshot) the
If someone have the tools and the platform, the following is still not tested:
|
Dragiyski
force-pushed
the
fix-inspector-filename
branch
2 times, most recently
from
2a0b2db to
8f67a90
Compare
bcoe
added
notable-change
|
I think we should probably call this a SemVer major, as upstream libraries tend to assert against error strings (which will change to having a |
There was a problem hiding this comment.
Thanks for the screen shots, I think this will be a definitely usability improvement for folks using inspectors.
Not for this PR, but two questions I'd like to eventually answer:
- how can we better handle
node://URIs. - how should we handle special paths like
[eval something].
| const line = call.getLineNumber() - 1; | ||
| let column = call.getColumnNumber() - 1; | ||
| let identifier; | ||
| let code; | ||
|
|
||
| if (filename && StringPrototypeStartsWith(filename, 'file:')) { | ||
| try { | ||
| filename = fileURLToPath(filename); |
There was a problem hiding this comment.
Might be worth getting a unit test around the edge case if possible 👌
|
@nodejs/modules @nodejs/tooling friendly nudge, it would be good to get a few more sets of eyes on this. It will alter stack trace output in a significant way. |
Dragiyski
force-pushed
the
fix-inspector-filename
branch
from
4139ef1 to
d30dbbf
Compare
Yes, this could solve the regression, but I am reluctant to do that, since In the latest commit I attempted another approach. I stopped using There is still significant regression, but not as severe as before. Here is the comparison to |
|
@Dragiyski Is this ready for review/landing in your view, or is there still some work you're hoping to do on it first? |
github-actions
bot
removed
the
request-ci
Yes, I think it is ready for review. |
|
I agree with @benjamingr's recommendation, let's benchmark a couple real world modules that are quite large, pulling in the index of googleapis would be a great candidate (its dependency graph is huge, and probably represents a good worst case). Ultimately, I'll trust to @joyeecheung and @benjamingr, as to whether the regression is acceptable. Otherwise, I'm happy with this functionality and am excited that it makes behavior more consistent between ESM and CJS 👍 |
|
@nodejs/tsc (Semver major so needs at least 2 TSC approvals.) |
|
I am still not sure whether making inspector stack traces nicer (a mostly dev-only feature) is worth the cost of the performance regression and potential breakage in the ecosystem. I think we should at least investigate how this would impact real-world packages. |
@joyeecheung @Dragiyski why don't we start with improvement in |
|
@bcoe Starting with the |
|
@Dragiyski can you please rebase on top of master to solve the git conflict? |
When the chrome inspector (chrome://inspect) display stack traces, it makes the filename clickable, and developer can easily locate the relevant source with a single click. This only works when filenames are valid URLs. The CJS loader seems to compile sources with absolute path instead of URL, breaking the inspector functionality. This is especially apparent when node_modules contain "at" symbol (@). The loader already presents module to the inspector using file URL, the same URL should be used for compiling the source.
It seems getErrMessage() function uses filename from a CallSite to open a file and extract source code location of the statement. The CallSite now returns file URL, so it must be converted back to path.
Various tests explore the stack trace or CallSite objects' filename. They are modified now to match file URL instead.
Windows path (using backslash) won't be found in stack trace anymore. Use appropriate url.fileURLToPath and url.pathToFileURL when examine the stack trace.
It seems `wrapSafe` filename is always absolute path. Certain aspects for validating paths in `pathToFileURL` can be skipped in favor of better performance of CJS synchrnous loading while keeping the origin.
Dragiyski
force-pushed
the
fix-inspector-filename
branch
from
f519f30 to
ca1ebbd
Compare
|
@Dragiyski please let me know when you have something that you'd like folks to take another look at. |
1 similar comment
|
@Dragiyski please let me know when you have something that you'd like folks to take another look at. |
|
This needs a rebase. |
|
@Dragiyski would be able to rebase on top of |
When the chrome inspector (chrome://inspect) display stack traces, it
makes the filename clickable, and developer can easily locate the
relevant source with a single click. This only works when filenames are
valid URLs. The CJS loader seems to compile sources with absolute path
instead of URL, breaking the inspector functionality. This is especially
apparent when node_modules contain "at" symbol (@). The loader already
presents module to the inspector using file URL, the same URL should be
used for compiling the source.
For example the current stack trace looks like this:
and as shown the link covers only the part after the first slash after the
@directory. The link leads to URL starting with slash (/), which opens an empty (about:blank) tab within the browser. This is an expected behavior, because in URLs@representsuser@hostdelimiter and the path is not valid absolute URL as it does not have protocol. A properly fixed URL looks like this:and clicking on the link provided in inspector opens the actual location of the source for all call sites, including node_modules containing
@symbol in their path. For the console, the path is printed withfile://in front and clickable in some terminals.Note: (...) replaces a valid path omitted for security reasons.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes