Review requested:

• @nodejs/modules

CI: https://ci.nodejs.org/job/node-test-pull-request/33777/

Codecov Report

Merging #35725 into master will decrease coverage by 0.00%.
The diff coverage is 89.47%.

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b6bbfe...f976a8b. Read the comment docs.

CI: https://ci.nodejs.org/job/node-test-pull-request/33993/

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?

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);
    });
})();

Output:
Error
    at Module.test (file:///home/dragiyski/projects/nodejs/test/error.mjs:2:11)
    at /home/dragiyski/projects/nodejs/test/main.js:6:21

New Output:
Error
    at Module.test (file:///home/dragiyski/projects/nodejs/test/error.mjs:2:11)
    at file:///home/dragiyski/projects/nodejs/test/main.js:6:21

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();
};

Output:
/home/dragiyski/projects/nodejs/test/error.js:2
    throw new Error();
          ^

Error
    at module.exports (/home/dragiyski/projects/nodejs/test/error.js:2:11)
    at file:///home/dragiyski/projects/nodejs/test/main.mjs:3:1
    at ModuleJob.run (node:internal/modules/esm/module_job:146:23)
    at async Loader.import (node:internal/modules/esm/loader:165:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5)

New Output:
file:///home/dragiyski/projects/nodejs/test/error.js:2
    throw new Error();
          ^

Error
    at module.exports (file:///home/dragiyski/projects/nodejs/test/error.js:2:11)
    at file:///home/dragiyski/projects/nodejs/test/main.mjs:3:1
    at ModuleJob.run (node:internal/modules/esm/module_job:146:23)
    at async Loader.import (node:internal/modules/esm/loader:165:24)
    at async Object.loadESM (node:internal/process/esm_loader:68:5)

As it can be seen, ESM modules already report local files as URLs. If any module on NPM examine CallSite object to retrieve a filename and use that filename to fs functions, it would break if it was included from ESM module. This will make the output consistent, so any new module would not need to check if CallSite is an URL (ESM, native node modules, etc) or absolute path (CJS), it would always be an URL.

P.S. It might be useful to allow using file: URLs in fs.

It looks like this needs some changes for tests to pass on Windows.

06:53:28 not ok 512 parallel/test-source-map-enable
06:53:28   ---
06:53:28   duration_ms: 0.664
06:53:28   severity: fail
06:53:28   exitcode: 1
06:53:28   stack: |-
06:53:28     node:assert:108
06:53:28       throw new AssertionError(obj);
06:53:28       ^
06:53:28     
06:53:28     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
06:53:29     + actual - expected
06:53:29     
06:53:29     + 'file://C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\fixtures\\source-map'
06:53:29     - 'file:///C:/workspace/node-test-binary-windows-js-suites/node/test/fixtures/source-map'
06:53:29         at Object.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-source-map-enable.js:90:10)
06:53:29         at Module._compile (node:internal/modules/cjs/loader:1087:30)
06:53:29         at Object.Module._extensions..js (node:internal/modules/cjs/loader:1116:10)
06:53:29         at Module.load (node:internal/modules/cjs/loader:948:32)
06:53:29         at Function.Module._load (node:internal/modules/cjs/loader:789:14)
06:53:29         at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:72:12)
06:53:29         at node:internal/main/run_main_module:17:47 {
06:53:29       generatedMessage: true,
06:53:29       code: 'ERR_ASSERTION',
06:53:29       actual: 'file://C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\fixtures\\source-map',
06:53:29       expected: 'file:///C:/workspace/node-test-binary-windows-js-suites/node/test/fixtures/source-map',
06:53:29       operator: 'strictEqual'
06:53:29     }
06:53:29   ...

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 fs tests on GitHub Actions CI on Windows because of something about the underlying file system that is escaping my memory right now.

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 fs tests on GitHub Actions CI on Windows because of something about the underlying file system that is escaping my memory right now.

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.

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).

1. I see you're updating the stack traces when source maps are enabled too (thanks for catching this). Does the inspector handle the linking appropriately for the original call sites added by --enable-source-maps?
2. I think we should take the time to make sure the paths are handled appropriately in the inspector UI on windows as well.

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.

1. I see you're updating the stack traces when source maps are enabled too (thanks for catching this). Does the inspector handle the linking appropriately for the original call sites added by --enable-source-maps?

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 http:// or https://) and it will be clickable even when the link is suffixed with colon (:) and a line number. If it matches registered script exactly it will jump to it, otherwise it will open a new tab in the last focused browser window. However this does not properly detects all URLs in general. For node modules node:internal/url.js will have only /url.js clickable. However, when printing a stack trace the entire part node:internal/url.js would be detected properly and it will be clickable. I have no idea why there would be any difference here. Even worse, when not all call sites are detected as URLs, call sites for node: are shown wrong (URL starts after the first slash), but when all call sites (that have filename) are URLs it works... 😕 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).

Anyway, the idea here is to register all scripts in v8::ScriptCompiler as URL, which is possible since v15.x. Node modules in lib are valid URLs with node: protocol. ESM modules are valid URLs with file:// protocol, even when they are loaded from the local filesystem. The only modules that fail to follow that format are the CJS modules. Because of the ESM modules all facilities to handle path to file URL conversion (or vice versa) are already present and used.

2. I think we should take the time to make sure the paths are handled appropriately in the inspector UI on windows as well.

I 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. The final goal is an URL and url module already provides suicient utilities for handling that logic even for more complex cases (detecting if URL is already absolute for any protocol). Originally I tried to apply minimal changes to address sources appended to the call stack, but as it is mentioned in #35903, the source could by absolute pseudo-URL from an external library (i.e. webpack://).

What I'd like to make sure, is that the inspector UI works as we'd hope on a few testing platforms:

• It would be good to have someone run an inspector session on Windows and confirm the same UX improvement.

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 --enable-source-maps: Screenshot
  • all file:// URLs are shown in full and clickable. Clicking open the containing file and it works for both the original and the mapped source. The file is scrolled to the specified line and the cursor is set to the column specified in the URL.
  • The node: URLs for native node modules are URLs after the first slash. Clicking on them does not lead to a source file, but opens about:blank in the nearest browser.
• without --enable-source-maps: Screenshot
  • All file:// URLs are now reduced to the a base name and still clickable. Even if source maps are not processed by the node, devtools process them and include the original source. In this case there is no link to the mapped source, only to the original specified in the source map. Links scroll to the proper line and put the cursor on the proper column.
  • All node: are fully shown and this time all the URL is clickable and lead to proper source file, line and column. Internal node modules are listed as (no domain) as expected and links open the specified file.

With both Webstorm (2020.2.3 on Ubuntu 20.04) (Screenshot) and VS Code (configuration above) (Screenshot) the file:// entire URL is visible and:

• with --enable-source-maps
  • All file:// URLs are clickable and open the referenced source file, line and column for both the original and mapped file.
  • Other URLs (including node:) are not clickable at all.
• without --enable-source-maps: same as above, but now since the lines for the original files are missing, there is only link to the mapped file. It does not seems those programs process source maps by themselves by default.

If someone have the tools and the platform, the following is still not tested:

• Mac OS
• v8::Function from a v8::FunctionTemplate on the stack trace.
• Negative scenarios:
  • Source maps with invalid URLs, URLs to non-existing original, Invalid source map data, etc.
  • Function compiled by vm with invalid URLs for a filename.

module benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/676/

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 file:// prefix).

CI: https://ci.nodejs.org/job/node-test-pull-request/34062/

@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 Is this ready for review/landing in your view, or is there still some work you're hoping to do on it first?

CI: https://ci.nodejs.org/job/node-test-pull-request/34208/

@Dragiyski Is this ready for review/landing in your view, or is there still some work you're hoping to do on it first?

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.

Yes, this could solve the regression, but I am reluctant to do that, since v8::ScriptOrigin would remain a path, not an URL, which in case of custom Error.prepareStackTrace would still result in inconsistent result between ESM and CJS modules (file URL vs path). If no other solution is found, this would do.

@joyeecheung @Dragiyski why don't we start with improvement in Error.prepareStackTrace, since it's less controversial?

@bcoe Starting with the Error.prepareStackTrace sounds good, since we are likely to be dealing with errors in those cases

@Dragiyski can you please rebase on top of master to solve the git conflict?

@Dragiyski please let me know when you have something that you'd like folks to take another look at.

@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 master to solve the git conflict please?

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.