Stop REPL stack traces at the REPL eval frame

[jakebailey]

Fixes #1259.

This makes errors thrown in the REPL look like this:

Cutting off all of the frames below the eval used to evaluate what was typed in the REPL, which can be arbitrarily large and is already implied by the debugger UI.

I don't have tests for this yet; I'm having trouble running them locally without a whole load timing out or failing because they expect __proto__ and I get [[Prototype]] or something.

[connor4312]

I don't have tests for this yet; I'm having trouble running them locally without a whole load timing out or failing because they expect proto and I get [[Prototype]] or something.

You probably have a mismatched version of Node.js locally -- V8 renamed that property a while ago. Usually I run npm run test:golden -- -g "name of test" to run a specific test

[jakebailey]

As in, this repo needs node 14? I had been using 16 because the repo uses npm lockfile v2 (otherwise I will break my node 14 install I use for TS, which is still on npm6), but I can just go back to 14 and leave my lock unclean while working.

[jakebailey]

The test failures are at least the ones I was expecting, so that's good. I'll fix things up.

[jakebailey]

There's still a failure in the console log tests because it looks like the eval source mapping overrides the one in the evaluated code. I'm not quite sure what to do about that.

Additionally, the new source file extension I invented is shown in traces that contain frames from the eval more than once (since the REPL can call back into itself and those frames are shown). I don't think that's ideal, but I'm not sure what to do about that one either. Making the extension be the .repl.cdp would hide them, but that's not very desirable when what I want is to hide things before the initial frame, not modify the trace in a significant way.

[jakebailey]

I am also questioning whether or not the initial eval frame should be printed or not; given it can show up thanks to self-referential code, it's probably more consistent to show it, but I still have to solve the extension name issue.

[connor4312]

Looking good to me overall, some comments

[jakebailey]

Making this a draft for now. I'm not exactly happy with the stack traces and how they show the faked filename.

Perhaps a better method would be to instead invoke evals as an IIFE with a well-known name?

[connor4312]

I'd be OK merging this PR, I tested it locally and I think it's looking good.

We're in full control of what filenames we show, what would you ideal solution look like?

[jakebailey]

Preferably, I'd like the path to look somewhat like node's REPL, e.g.:

> eval("throw new Error('hey')")
Uncaught Error: hey
    at eval (eval at <anonymous> (REPL1:1:1), <anonymous>:1:7)

Where the filename is just "REPL" or something, versus how this PR talks about localhost and stuff. The browser is a little different, instead showing the page name + :1:1:

VM24:1 Uncaught Error: hey
    at eval (eval at <anonymous> (performance:1:1), <anonymous>:1:7)
    at <anonymous>:1:1

But, I'm not sure exactly the right thing to do. I already don't like the fact that this method of identifying the eval overrides any user provided source map for the REPL, but I suppose nobody's putting their own source mapping into REPL statements.

[connor4312]

Preferably, I'd like the path to look somewhat like node's REPL

You can edit that here 🙂

vscode-js-debug/src/adapter/sources.ts

Lines 332 to 340 in 98b20c3

	/**
	* Returns a pretty name for the script. This is the name displayed in
	* stack traces and returned through DAP if the file does not verifiably
	* exist on disk.
	*/
	private _fullyQualifiedName(): string {
	if (!this.url) {
	return '<eval>/VM' + this.sourceReference;
	}

I already don't like the fact that this method of identifying the eval overrides any user provided source map for the REPL, but I suppose nobody's putting their own source mapping into REPL statements.

It actually doesn't -- it would override the url, but not the map. It's already quite unlikely for a sourcemap with relative external references to work (which is what the url would affect) but a fully inline sourcemap would still work.

[jakebailey]

Aha, thanks. I'll change that to switch it to repl or something.

[jakebailey]

Unfortunately that's not the right thing; stack trace printing actually looks directly at Source.url if it's present (and a lot more code). I can modify the stack trace code to ignore that if it's a repl, though, since that should be the only place that this is observable, but I find it surprising that formatAsNative prefers the url over the pretty name.

[jakebailey]

I've changed it so that you get repl: in the trace. To do this, I made formatAsNative not check url; maybe that's wrong but the difference feels superficial. You can see the difference in the golden files.

However, I get this error when I run the tests (this has probably been true the whole time):

rejected promise not handled within 1 second: TypeError: Cannot read properties of undefined (reading 'endsWith')
stack trace: TypeError: Cannot read properties of undefined (reading 'endsWith')
        at Function.fromDebugger (e:\work\vscode-js-debug\out\src\adapter\stackTrace.js:187:127)
        at Function.fromDebugger (e:\work\vscode-js-debug\out\src\adapter\stackTrace.js:83:44)
        at Thread._createPausedDetails (e:\work\vscode-js-debug\out\src\adapter\threads.js:775:52)
        at Thread._onPaused (e:\work\vscode-js-debug\out\src\adapter\threads.js:544:59)
        at runMicrotasks (<anonymous>)
        at processTicksAndRejections (node:internal/process/task_queues:96:5)

The protocol says that url in this case must exist and be a string; something upstream is breaking that invariant. Maybe some test that is casting?

[connor4312]

When you run the tests, on a failure, it should print the location of a .json file. Could you share that? That'll have more information.

[jakebailey]

Sorry for the delay; was OOF. Here's the JSON file (with extension txt so GitHub lets me upload it):

node-runtime-chakracore-string-value.txt

[jakebailey]

These changes are due to me dropping uiLocation?.source.url use from formatAsNative so the pretty name shows up instead.

[jakebailey]

@connor4312 I have merged main and simplified a few things; I don't know that the above error happens anymore locally on my machine, but it doesn't look like Pipelines is running on my PR anymore so I can't tell if I'm still breaking something on Windows. I'm thinking the undefined bug got fixed sometime this month.

This stack trace thing is annoying me quite a bit so I'm hoping to see this one go in.

[connor4312]

lgtm