Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve discoverability of js-debug diagnostic tool #57590

Closed
weinand opened this issue Aug 30, 2018 · 21 comments
Closed

Improve discoverability of js-debug diagnostic tool #57590

weinand opened this issue Aug 30, 2018 · 21 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Aug 30, 2018

Today I saw another problem with breakpoints not being hit because the sourcemap's sourceRoot was wrong (because it was configured incorrectly in the tsconfig.json).

The fix was trivial for me, but for a user it is not obvious what the problem is. Hovering over the grey breakpoint just shows a not very informative message "unverified breakpoint".

And not many users are aware of the ".scripts" utility that can be run on the debug console to see the source mappings in detail. And the output of ".scripts" is a bit verbose and does not highlight errors. See this example from node-debug which has a wrong sourceRoot entry in the source map of sourceMaps.js:

2018-08-30_11-33-33

I think we can and should do better!

My proposal:

For obvious reasons we cannot show a warning message when an expected breakpoint is not hit.
But we could do an upfront verification pass over all generated files reachable via the outFiles globs and if they have a source map reference at the end we can verify that the source map exists and that contained back reference to the source is points to an existing file.
If the verification process sees error, we could show detailed information about the problem.
In order to keep the amount of information small, we could limit the verification to only those source files that actually have breakpoints.

This verification can be implemented in basically three ways:

  • inside the DA in "launch" or "attach" when the launch config is available. The output of the verification could be send to the debug console in text form and we could either treat problems as fatal and abort debugging or as informative and continue debugging. The verification could use the ".scripts" tool and verify the paths in its output and only show the errors.
  • inside the node-debug extension: here we could verify the launch config and the source maps in the resolveDebugConfiguration method which makes it possible to show nicer UI and ask the user whether he wants to continue or abort debugging. Another advantage of doing the verification in the extension code is that the code is shared for both node-debug and node-debug2.
  • inside a separate extension. This approach could be useful for any debugger that uses sourceMaps. But since launch configs are not standardized, it is difficult to find and interpret the relevant attributes correctly.

@roblourens what do you think?

/cc @dbaeumer

@weinand weinand added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Aug 30, 2018
@weinand weinand self-assigned this Aug 30, 2018
@weinand weinand added this to the On Deck milestone Aug 30, 2018
@roblourens
Copy link
Member

I think this would have to happen inside the DA, because we would need to verify this after sourcemaps are loaded or downloaded, (they aren't always available through outFiles) and after pathMapping and sourceMapPathOverrides have been applied.

Also I think it shouldn't cause debugging to stop. Files can be loaded at any point during the execution of the program, we can't do all of this up front. And there are probably people who have some files that aren't resolved correctly, but they are still debugging successfully, and we shouldn't disturb them too much. There are probably some valid scenarios like when you are debugging and only have part of the project checked out locally.

I think that printing a warning could be useful, when a script loads and its sourcemap paths are not resolved to a file on disk. As long as it's not spammy.

As a first step, we could add some telemetry for that case - it would be interesting to see how many people are debugging with unresolved sourcemaps. Also, whether breakpoints are actually resolved to locations, I don't think that's covered by telemetry.

@justingrant
Copy link
Contributor

justingrant commented Mar 19, 2019

Making it easier to troubleshoot and fix sourcemap issues is the number one thing that VS Code could do to make my work more efficient. I've wasted more time dealing with sourcemaps than any other part of my Node/React app. So help in this area would be very useful!

I think a good starting point could be to add a debug console command that provides scriptmap diagnostics that are easier to use than the current .scripts command output. Pointing people to this command (to make it more discoverable than .scripts today) would be helpful, e.g. in a message in the debug console when a session starts.

Here's some suggestions for requirements for these diagnostics:

  • It's included in intellisense, so that you don't have to hit ESC before pressing ENTER to run it. It's really annoying, in my 20th time running .scripts today, to always have to defend against running ScriptProcessorNode instead!
  • By default only errors should be shown, where "error" means that the file doesn't currently exist on disk. A path that's successfully mapped (e.g. with sourceMapPathOverrides) but where the file doesn't exist is still an error, and today with .scripts it's very hard to know which mappings point to paths that don't exist.
  • When there's an error, it should show more info than .scripts does:
    • Original path in the sourcemap, e.g. '../source/foo.js'
    • Mapped path (if there was a mapping), or "(empty)" or "(none)" or some other greppable string to identify completely missing mappings. Today with .scripts, the non-mapped path is just repeated which is not helpful.
    • The full path of the sourcemap file where the path was found, which can be helpful when trying to help libraries to fix their broken sourcemaps
    • Where the mapping came from, e.g. the specific line in sourceMapPathOverrides
  • In addition to looking at loaded scripts, it should also scan all packages in node_modules and identify .map files where the corresponding source isn't present, even if those packages are not currently imported in the current debug session. This should even work even if a debug session is not active, so you could proactively find problems before getting knee-deep into a debug session. It'd also make it easier to provide repro steps for a library author by removing the need to have a debuggable app.
  • It should be easier to get its output into a code window. Today capturing the output to .scripts requires several non-intuitive steps, and even copy/paste works oddly inside that pane. Ideally it'd be a one-click operation to open a new file with the output, which you could then copy/paste into Excel or some other tool for filtering and categorizing the issues.
  • Making CMD+F work inside the debug console would be vey helpful!
  • Ideally whatever diagnostics is used inside VSCode would be wrapped in an npm packge that could be re-used by other tools, e.g. np that might want to help developers diagnose sourcemap issues.

Note that most of the sourcemaps issues I run into are caused by libraries, not by app code. There are two basic reasons:

This means that troubleshooting inside VSCode is an important first step, but solving the problem also involves getting lots of library authors to fix their sourcemaps. IMHO the ideal would be a warning inside npm publish that would warn library authors before they published a broken sourcemap to npm.

@roblourens
Copy link
Member

It's included in intellisense,

Agreed, if anyone wants to work on this, I would definitely take a PR for this for .scripts

By default only errors should be shown, where "error" means that the file doesn't currently exist on disk.

I would take a PR for this, of course finding a file at that path doesn't mean the mapping is correct, and not finding a file at that path may or may not be an error. e.g. inline sources may be expected to be used. Or they may be present but the file is expected to be present.

Original path in the sourcemap, e.g. '../source/foo.js'

I think it currently shows sourceRoot + source

Mapped path (if there was a mapping), or "(empty)" or "(none)" or some other greppable string to identify completely missing mappings. Today with .scripts, the non-mapped path is just repeated which is not helpful.

I don't think this is technically an error but I'm not sure

The full path of the sourcemap file where the path was found, which can be helpful when trying to help libraries to fix their broken sourcemaps

👍

In addition to looking at loaded scripts, it should also scan all packages in node_modules and identify .map files where the corresponding source isn't present, even if those packages are not currently imported in the current debug session. This should even work even if a debug session is not active, so you could proactively find problems before getting knee-deep into a debug session. It'd also make it easier to provide repro steps for a library author by removing the need to have a debuggable app.

But this could be a useful separate tool for some people. It sounds like it would make sense as a tool separate from vscode.

It should be easier to get its output into a code window. Today capturing the output to .scripts requires several non-intuitive steps, and even copy/paste works oddly inside that pane. Ideally it'd be a one-click operation to open a new file with the output, which you could then copy/paste into Excel or some other tool for filtering and categorizing the issues.

Right click to Copy All? (Not a perfect solution though)

@petermetz
Copy link

Having similar issues in a setup where I'm using webpack with TS loader to generate server side bundles of node apps with inline source maps.

@weinand
Copy link
Contributor Author

weinand commented Nov 5, 2020

@connor js-debug has already something like this, correct?

@weinand weinand assigned connor4312 and unassigned weinand Nov 5, 2020
@connor4312
Copy link
Member

Yes, we have a diagnostic tool as of microsoft/vscode-js-debug#260, but currently this is not discoverable. I will use this issue and some of your suggestions for that.

@connor4312 connor4312 changed the title Verify source maps Improve discoverability of js-debug diagnostic tool Nov 5, 2020
@connor4312 connor4312 modified the milestones: On Deck, November 2020 Nov 5, 2020
@connor4312 connor4312 modified the milestones: February 2021, March 2021 Feb 23, 2021
@connor4312 connor4312 modified the milestones: March 2021, April 2021 Mar 24, 2021
@tomatac
Copy link

tomatac commented Mar 28, 2021

Is there a js-debug diagnostic tool? I am getting 'unbound breakpoint' when I run the debugger for a react app and I cannot figure out what is wrong. The same configuration worked ok few weeks ago.

@tomatac
Copy link

tomatac commented Mar 29, 2021

I found Create Diagnostic Information command. Great tool! It would be nice to be better documented.

@connor4312
Copy link
Member

I've decided to try out showing a helper prompt when all of these conditions are met:

  1. At least one breakpoint was set, but no breakpoints bound,
  2. For two consecutive debug sessions,
  3. Where a sourcemap was used for a script outside of the node_modules, or a remoteRoot is present (since sourcemaps and remote are the cases where almost all path resolution issues happen)

connor4312 added a commit to microsoft/vscode-js-debug that referenced this issue Apr 5, 2021
@connor4312
Copy link
Member

connor4312 commented Apr 5, 2021

This happens five seconds into the subsequent session (the 3rd session) given breakpoints are still unbound:

recording (2)

edit: looks like the gif has some odd graphical glitches, but you get the idea

@justingrant
Copy link
Contributor

@connor4312 - This looks great! Thanks so much for moving this forward. Some questions:

image

Can this text contain a link to the docs where the user can learn how to do this? Also, other than changing the build tool (which may not be possible in some cases, e.g. non-ejected create-react-app), the other option is to use sourceMapPathOverrides in launch.json. Seems like this option should either be noted here or in the docs that are linked from here.

3. Where a sourcemap was used for a script outside of the node_modules, or a remoteRoot is present (since sourcemaps and remote are the cases where almost all path resolution issues happen)

In my experience, the most common sourcemap problem I run into is when libraries in node_modules were built or published incorrectly so that they contain sourcemaps but don't publish the source files corresponding to that sourcemap or when the library's sourcemap points to a different path the published source files.

The ideal behavior from VSCode (which might not be possible) would be to identify that
a) the problem is caused by a library, not the user's own code
b) the problem is that the source files weren't published or were published in a different place than the sourcemap points to
c) the best way to fix it is to open an issue or PR against the library (if not, the only solution is to work around the issue, e.g. using sourceMapPathOverrides)
d) here's a link to open an issue in the library's repo, and here's some text you can copy into the issue to help the maintainer to understand what's the problem and why it's important to publish source files to npm.

Is this possible? I'd be happy to help with the extra text required.

@connor4312
Copy link
Member

Thanks for the feedback! I'll tweak things. I'm also happy to take PRs, the source code for the breakpoint helper is here. Simple small (p)react app with a handful of heuristics 🙂

@jrieken
Copy link
Member

jrieken commented Apr 21, 2021

This is very confusing and I don't understand why I am seeing this notification. All I am doing is debugging integration tests (with success) when running out of sources. So

  • run ./scripts/code.sh <vscode-folder>
  • run API test launch config and debug some API/integration test failure
  • 😕 you get the notification after a debugging/working for a bit

Screenshot 2021-04-20 at 14 37 10

@connor4312
Copy link
Member

@jrieken do you have (no breakpoints in api tests and) breakpoints set in core that are not hit with the extension host config? That's a known case where the helper can display inappropriately -- if you have a workspace with multiple disjoint configs.

@connor4312
Copy link
Member

I found a bug this morning that can cause this to pop up when it shouldn't. Will be better in the next build.

@jrieken
Copy link
Member

jrieken commented Apr 21, 2021

@jrieken do you have (no breakpoints in api tests and) breakpoints set in core that are not hit with the extension host config

No, only breakpoints in the API tests, like notebook.test.ts

@jrieken jrieken reopened this Apr 23, 2021
@jrieken
Copy link
Member

jrieken commented Apr 23, 2021

I am sorry but isn't working well and just annoying. Now I see this when attaching to the extension host and when doing "normal" debugging

Screenshot 2021-04-23 at 08 46 03

@connor4312
Copy link
Member

@jrieken are you on nightly or stable? I haven't released a new stable build with the fixes yet.

@jrieken
Copy link
Member

jrieken commented Apr 23, 2021

idk - whatever ships with insiders

@connor4312
Copy link
Member

Okay, yea I see on the notification it's not nightly. I will bump js-debug this afternoon or Monday, at which time this should be fixed.

@connor4312 connor4312 added verification-needed Verification of issue is requested verified Verification succeeded labels Apr 27, 2021
@connor4312
Copy link
Member

connor4312 commented Apr 27, 2021

Marking verified since it can't really be verified for endgame as it's in an ongoing experiment

@github-actions github-actions bot locked and limited conversation to collaborators Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants