microsoft / vscode-js-debug

A DAP-compatible JavaScript debugger. Used in VS Code, VS, + more
MIT License
1.67k stars 281 forks source link

Allow to pick exact tab to attach to if several tabs match urlFilter #479

Closed IllusionMH closed 4 years ago

IllusionMH commented 4 years ago

During my work I usually have several pages that will match urlFilter e.g. *example.com/app*:

http://client.staging.example.com/app/orders/order1
http://client.local.example.com/app/orders/order1
http://client.local.example.com/app/orders/order2
http://search.local.example.com/app/results

So I can compare behaviors/outputs/request between them and having only one tab open is not an option.

At this point when I run debug config with "type": "pwa-msedge", "request": "attach", it will attach to "random" tab that matches urlFilter.

Current Edge/Chrome debugger extensions provide ability to pick exact tab before attaching to it. Edge example

Would be great to have similar approach using built-in extension.

I was thinking that #437 will allow this, but apparently it does something else.


Version: 1.46.0-insider (user setup) Commit: 6849775440496504e0677ffcf2a46d47ad9130f5 Date: 2020-05-15T05:30:56.857Z OS: Windows_NT x64 10.0.18363

JavaScript Debugger (Nightly) Built-in v2020.5.417

connor4312 commented 4 years ago

Alternatively, we could attach to all of them with the model in js-debug (where the previous debugger had to attach to exactly one tab). Thoughts?

IllusionMH commented 4 years ago

Frankly speaking I not sure how it should behave when you say "we could attach to all of them with the model". It's clear how it works when I'm in multi-target debugging and have Node and Edge debugging sessions in parallel, because steps/breakpoints will be hit in different sets of files.

However in case of 2 tabs http://client.local.example.com/app/orders/order1 and http://client.local.example.com/app/orders/order2 which will execute same code but differ only in some data inside of variables it will be confusing to differentiate them if same breakpoints will be hit from different tabs in various async handlers. At least I don't have any idea how it can be done efficiently.

Approach with list at the beginning of session is simple/intuitive. After start it might be useful to have same list possible targets and ability to quickly switch to debugging another tab (especially in case of multitarget debugging).

In my case Detach and then run Attach + pick another tab from list works fast/simple enough.

connor4312 commented 4 years ago

Actually, as of https://github.com/microsoft/vscode-js-debug/issues/437 (the built in version is a little behind) we attach to all tabs that match the url filter. You can detach from pages by clicking the "disconnect" button if you don't want to debug them. I think I would prefer this approach over a list, since a list requires an extra user interaction on each launch, and I can narrow my urlFilter if I want to exclude pages from being hit.

I've updated the version of js-debug in Insiders, let me know what you think of it.

IllusionMH commented 4 years ago

I manually picked 2020.5.1917 version and it is worse experience for me in comparison with Debugger for Microsoft Edge extension:

  1. List shows last part of URL and 10 UUIDv4 aren't helpful, while page titles would be different. No title or any other hint to differentiate between same pages on different hosts (local, staging). Tooltip will be still worse experience than pick list. In Edge/Chrome debugger has page title as label and full URL as detail.

  2. Reloading any tab wipes previous logs in debug console even if they are from different tab No problems with single target.

  3. Additional 2 levels of nesting in Call Stack, not expanded to tabs list by default. List seems random on each session start (Disconnect/Run) so need to reload tabs with similar URL ends to find one that I actually want to debug each time.

  4. Debug bar feels useless:

    1. Of course Disconnect disconnects only 1 session so I need to click several times or use Disconnect in Call Stack

    2. Two tabs paused on same debugger statement? Now it's just guess game what tab will be released, because it depends on what tab was paused later. Need to use actions for each tab instead.

    3. Mix of opened call stacks makes it had to find required tab to use action on it.

    4. Restart button on "Attach to PWA Edge" in Call Stack does nothing or it's confusing what it should do, because same Restart on tab items reloads page

  5. Even if disconnected from tab (have only 1 tab left) - any navigation that changes URL in that tab will bring it back.

  6. (Occurred couple of times, hard to repro) Having debugger; somewhere in code makes other reloaded tabs unusable - HTML is loaded but not requests for JS files in browser DevTools, probably stopped on inline script (I wasn't able to check with breakpoints because they don't work - even when I have only 1 tab opened, current extension version in Insiders, config identical to one for edge)

Hashes are picked with last part of the URL not sure if it's a feature. Both "debug session" and "tab" tree items have tooltip "Session" which isn't helpful.

On the other hand existing Edge/Chrome extensions had pick list for a long time and that strategy worked well at least for me:

  1. Easy to pick target - Page title + full URL, fast enough to switch to other target
  2. Debug toolbar is single source of control
  3. Only 1 call stack so won't be confused between tabs. Even in case of multi-target debug I hand maximum 2 stacks.
  4. No need to disconnect from other sessions to debug single tab (which still doesn't work as described above)

So in my opinion current implementation may be a nice options for those who want it, but makes interaction with debugger worse with little to no benefit. Good old Pick list at session start is a good default or at least should be available as an option.

@connor4312 could you revisit your decision and reopen this request?

connor4312 commented 4 years ago

Thanks for the great feedback! I have added an options to chrome attach back: targetSelection: 'pick' | 'automatic', where setting it to "pick" will open the quick pick.

But it is worth looking into the other issues you mentioned for others who might intentionally debug multiple pages:

  1. I found a Chromium bug previously which means that we sometimes are not told when the page title changes https://bugs.chromium.org/p/chromium/issues/detail?id=1058108&q=targetInfoChanged%20title&can=2 but it looks like we weren't trying to use the page title if the url is valid. I have made a change so that the page title is the default target title, and the page URL is displayed to disambiguate pages if there are two targets with the same name/title. For example, here I have two pages open with the same title:

    I've also added a context menu action to focus a tab in the browser from the call stack view:

    1. Because we merge terminals, I don't think there's a good way to handle this. We can't later terminals if we see multiple pages, and dealing with multiple consoles in general may be difficult. For now I have removed the line that clears on refresh. I personally found this annoying even for general single target development.

    2. Should be fixed with 1

    3. These are things that have been brought up before, and I agree there can be confusing cases. There is a focused session in the call stack view, and the debug toolbar controls whichever session is currently selected. The restart button is something that, as a debug adapter, we cannot currently control the presence of, even though some targets have restart as a no-op.

    4. We could deny-list the tab from being reattached after you detach from it. But imagine you have a long-lived browser in debug mode -- remembering the 'banned' tabs indefinitely might lead to confusion if you try to pull up the project in a tab you previously disconnected from. Maybe we remember the ban until the user navigates off of their debugged application, or maybe this isn't an issue at all. Thoughts?

    5. Can you enable "trace": true and try to get into one of these scenarios?

IllusionMH commented 4 years ago

I have added an options to chrome attach back: targetSelection: 'pick' | 'automatic', where setting it to "pick" will open the quick pick.

Thanks! This one looks great as with old debuggers.

I have made a change so that the page title is the default target title, and the page URL is displayed to disambiguate pages if there are two targets with the same name/title.

Definitely helpful! An addition I would suggest to add full document title and URL to target title (on hover), because it would be easily cropped by actions and now all have "Session" as title. image

Focus Tab is also nice addition.

Should be fixed with 1

Yes, 1 should help in case of multi-tab debugging. In case when "targetSelection" is set to "pick" is it possible to remove 2 levels nesting (Launch config name and Tab title) which look redundant, but not a (big) problem. image Edge debugger extension for comparison image

Maybe we remember the ban until the user navigates off of their debugged application, or maybe this isn't an issue at all. Thoughts?

Personally I think that banning tab/debug session by ID is OK, but that's me. That was an issue when I tried to emulate single tab tab debug, but may be not an issue for those who's looking for muilti-tab debug. Probably better to leave it for now and see if there will be other feedback around this one.

Can you enable "trace": true and try to get into one of these scenarios?

Wasn't able to block one tab with debugger in another after that cases. I guess somehow Edge/Debugger get into bad state after several cycles of attach/detach and several Edge windows with a lot of tabs.

However I noticed strange behavior that not reproducible with Debugger for Microsoft Edge: If I have browser DevTools open, attach debug session form VS Code, and reload page - there are couple of times when I see "Paused in Debugger" (also highlight on first line of bundled file in browser DevTools) but it is "released" by itself in couple of seconds.


Also - is there a way to verify/debug source maps lookup and sourceMapPathOverrides resolutions?

Same config as for Chrome/Edge doesn't work, but I can't find enough info to report separate issue.

connor4312 commented 4 years ago

An addition I would suggest to add full document title and URL to target title (on hover), because it would be easily cropped by actions and now all have "Session" as title.

Feature request linked ^

is it possible to remove 2 levels nesting (Launch config name and Tab title) which look redundant

This has been the topic of a few discussions on various issues and internally. We'd like to do something here, but this is difficult at the moment due to some edge cases. In this specific case it's technically possible, but removing the top-level session is a structural change that I don't want to take for a single case.

If I have browser DevTools open, attach debug session form VS Code, and reload page - there are couple of times when I see "Paused in Debugger" (also highlight on first line of bundled file in browser DevTools) but it is "released" by itself in couple of seconds.

This is related to how we handle sourcemaps; we ask Chrome to pause before it evaluates a script with a sourcemap so that we can set breakpoints in it. On my todo is write some more documentation on how js-debug works for referencing 🙂

Also - is there a way to verify/debug source maps lookup and sourceMapPathOverrides resolutions?

Similar to #260

IllusionMH commented 4 years ago

This is related to how we handle sourcemaps; we ask Chrome to pause before it evaluates a script with a sourcemap so that we can set breakpoints in it. On my todo is write some more documentation on how js-debug works for referencing 🙂

This one makes it's really problematic to have both debug session and browser DevTools (for React DevTools or Network): 1) Constant switches to Sources tab (need to switch back to network/React tools each time) and pause(devtools and page overlay) blinking (at least once for each file with breakpoint even if all disabled, sometimes more) I even created stroboscope effect when experimented on page with a lot of modules and wrong source maps mappings.

2) It really affects time to load/render page. In some cases noticeable lag in rendering parts of the page (most likely due to pauses in bunch of the files) even when I disconnect debug session.

But how and why it's different from Debugger for Edge/Chrome extension that doesn't have this problem?

I think this scenario should be considered/addressed before making this extension default debugger for everyone.


As for source maps - apparently this debugger can't handle

"sourceMapPathOverrides": {
    "webpack:///./*": "${webRoot}/dist/client/*",
    "webpack:///../*": "${webRoot}/dist/*"
}

Having only "webpack:///./*": "${webRoot}/dist/client/*" or "webpack://?:*/*": "${webRoot}/*", will work for files nested inside of client folder, but files 1 level above (page wrappers, inlined/patched third party libs) aren't accessible and attempt to place breakpoints inside them will place id somewhere randomly in lib files Having both - looks like only files outside of client will be mapped correctly. image

Not clear why they don't work as before, because it doesn't look like new URL(leftPattern).toString(); has any effext

IllusionMH commented 4 years ago

Apparently Node.js and browser has different behavior for new URL(leftPattern).toString() and now both result in 'webpack:///*'. I don't think it's expected behavior.

Should I create new issue for this case or there are other "expected" workarounds?

connor4312 commented 4 years ago

Thanks for letting me know. The source-map library helpfully "normalizes" the source URLs for us which clobbers relative patterns. I've introduced some logic to preserve the original source paths and map between them.

IllusionMH commented 4 years ago

I've introduced some logic to preserve the original source paths and map between them.

Thanks I was able to have "webpack:///../*": "${webRoot}/*" for simple repro case 👍, but doesn't work for main large project.

Will try investigate further and create new issue with details.

connor4312 commented 4 years ago

Sounds good. One way to diagnose path issues is adding a debugger; statement to your code, and then when it's hit you can see the path of the readonly file that gets opened -- that's the path that js-debug thinks your source should be at.