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

Improvements to Node Auto Attach to improve defaults and discoverability #53640

Closed
auchenberg opened this issue Jul 6, 2018 · 18 comments
Closed
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code

Comments

@auchenberg
Copy link
Contributor

auchenberg commented Jul 6, 2018

We should improve Node auto attach in the following ways:

  1. Remove the Toggle Auto Attach status bar action, and replace it with the user setting instead. It's my understanding that the StatusBarAction was added due to a performance concern on Windows, which have been resolved.

The rationale is to make it a global setting, and declutter the already full statusbar. This would also enable a workspace setting to override for a specific project, and enable users to toggle this once.

  1. We should enable Auto attach by default, and provide more guidance to the user to on what's going on. For example, show a notification when a debug command line is detected in the terminal, that has an explainer text, and an action to disable.

The overall rationale is to improve discoverability and provide a more opinionated default.

cc @egamma, @weinand

@auchenberg auchenberg added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Jul 6, 2018
@weinand
Copy link
Contributor

weinand commented Jul 6, 2018

@auchenberg

  • the performance concern on Windows hasn't been solved (because the Auto-Attach features lives in an extension and we cannot use Daniel's process-tree module because it uses native code).
  • I agree that using a setting for this feature is the right approach. But that would require that the node-debug extensions are activated on startup (which is a bit strange for users that are not using node.js). We could solve this issue if there were an activation event that fires on a specific user/workspace setting. Without such event the only workaround I can think of is to move auto-attach into another built-in extension...

@weinand
Copy link
Contributor

weinand commented Jul 6, 2018

I will create a new built-in extension to address the settings issue.

@weinand
Copy link
Contributor

weinand commented Jul 13, 2018

The auto-attach is now a setting which makes it survive restarts.

I will not enable it by default for these reasons:

  • polling for all processes is still slow on windows and we cannot use the optimised 'process tree' module because it uses native code,
  • continuous polling is a waste of resources if a user has no interest in node.js,
  • currently we cannot differentiate whether node was launched in the integrated terminal because the user typed a command or whether a launch config started node.

@auchenberg
Copy link
Contributor Author

@weinand What will happen in case 3) today if you have auto-attach enabled?

@weinand
Copy link
Contributor

weinand commented Jul 16, 2018

@auchenberg if you are using F5 on a launch configuration configured for the integrated terminal, you will get this dialog after 10 secs:

2018-07-16_14-57-47

@weinand
Copy link
Contributor

weinand commented Aug 6, 2018

I've introduced a new "autoAttach" setting that survives debug sessions but we cannot turn this on by default because it interferes with debug sessions that are started via launch configurations and run in the integrated terminal.
In this case the "auto attach" mechanism does not know whether a new node.js process has been started manually from the command line or via a launch config.
This issue is deferred until we have found a way to differentiate those cases.
This problem is covered by issue #49403.

@weinand weinand modified the milestones: August 2018, On Deck Aug 26, 2018
@Tyriar
Copy link
Member

Tyriar commented Aug 30, 2018

@weinand you mentioned you were planning on using onDidChangeActiveTerminal, activeTerminal and onDidWriteData but didn't go into details on how you wanted to use them. Were you thinking of tracking onDidWriteData of the activeTerminal to scan for the command or something? If so I would recommend against this as onDidWriteData can cause a lot of traffic/slowness that we really don't want enabled by default. Also how would you differentiate user input from text sent from the debug part?

Some other options:

  • Do you have access to the launch.json terminal's PID that you could blacklist? (are they in the same extension?)
  • You could give the launch.json terminal a special name and blacklist that in auto attach?

@markm77
Copy link

markm77 commented Jul 26, 2020

Hi there! I just wanted to ask how you guys would feel about extending auto-attach to Node processes more generally (e.g. in my case those launched by C#). I use a library to make calls from C# to TypeScript (JavaScript) exported functions and unfortunately the JavaScript debug flow with VS Code is a real pain. It's easy enough to make calls from C# to Node use "--inspect-brk" but then for every single call I need to manually attach in VS Code.... I also have to do it not too early which usually means adding a C# breakpoint at the C# call site. Once I hit the C# call site breakpoint the flow is as follows: (a) press F5 (continue) in C# debugger to start call to JavaScript, (b) Attach VS Code debugger to Node process, (c) press F5 (continue) in VS Code debugger to move on from unwanted entry breakpoint and finally get to my TypeScript breakpoint.

I get a much better experience in JetBrains Rider where I can use an "Attach to Node.js/Chrome" debug config which crucially includes an option to "Reconnect automatically". I can just leave this debug config on and it always picks up calls from C# to JavaScript allowing me to set and hit both C# and TypeScript break points without having to mess around at the C# to JavaScript call site (there is a noticeable delay for re-connect though). Rider also (at least by default) doesn't stop at the entry breakpoint implied by "inspect-brk" on every JavaScript call which is great.

However I would actually rather do TypeScript/JavaScript debugging in VS Code than Rider so just wondering if you guys might have any thoughts on a solution before I just come up with some feature request I think of?? Or maybe there is a better way to do this??

@weinand
Copy link
Contributor

weinand commented Sep 9, 2020

@markm77 could you please turn your comment into a new feature request. That makes it easier to start a discussion.

@connor4312
Copy link
Member

connor4312 commented Sep 9, 2020

Also, @markm77, in the upcoming release of VS Code (1.49), auto attach has been reworked. The scenario you're describing will work out of the box without needing to pass --inspect flags, if the Node process you spawn inherits the environment variables from the running process/terminal (auto attach is started by hooks in the environment).

@markm77
Copy link

markm77 commented Sep 9, 2020

Thanks @weinand and @connor4312 for comments. Perhaps I should try the new release to work out what works/doesn't in my case and then if necessary make a new feature request? @connor4312 are you suggesting I can set an environment variable in e.g. Rider C# debug config and provided that is propagated VS Code will then auto-attach to a launched Node process? And without --inspect-brk? That sounds pretty nice.

Edit: or is this limited to processes launched by VS Code and its terminal?

@connor4312
Copy link
Member

This is limited to processes launched from VS Code's integrated terminal. Any Node process launched there, directly or indirectly, can be debugged with auto attach.

@markm77
Copy link

markm77 commented Sep 9, 2020

Okay, well I'll definitely try it out. My use case would be to run a C# unit test that spawns a Node.js/JavaScript call. I'll see if there is a nice way to run a C# unit test (Xunit) in VS Code that triggers your hooks. I'll report back. 😀

@connor4312
Copy link
Member

connor4312 commented Sep 9, 2020

I don't know much about the C# debugger, but if it can run the test in an integrated terminal (in the js debugger this is an option "console": "integratedTerminal", versus its default of launching a subprocess) then auto attach will work.

@markm77
Copy link

markm77 commented Sep 9, 2020

Thanks @connor4312 , there's what you mentioned but also the user experience for launching C# tests since I'll be doing this a lot and normally it's best to use some kind of test runner UI to select and run individual tests (rather than find them in the code each time). I think I need to see if I can find a good C# test runner for VS Code that launches tests in a way compatible with your hooks.... At the moment I use Rider as my C# test runner but I'm happy to test a VS Code C# test runner to allow debugging JavaScript part of a test in VS Code.

@markm77
Copy link

markm77 commented Nov 14, 2020

Apologies for the late feedback..... but here it is....

I did test with what seems to be the standard .NET/C# test runner (https://marketplace.visualstudio.com/items?itemName=formulahendry.dotnet-test-explorer). This runner is quite buggy/feature-deficient unfortunately but I couldn't see any other alternative with decent reviews/install count so continued with it.

I set debug.javascript.autoAttachFilter to always but couldn't intercept the Node.js process (with breakpoint directly in JavaScript). I guess this is because it is spawned by the C# process rather than directly via a terminal command? I couldn't find any extension settings to change how the C# process is spawned but assume anyway this is not the issue.

I don't know if you would consider intercepting Node.js processes launched externally (given the lack of a good C# test runner in VS Code) but I'm happy to create an issue for this (I'll do it shortly and post a note here).

Thanks for your helpful comments.

@connor4312 connor4312 modified the milestones: January 2021, On Deck Jan 26, 2021
@connor4312 connor4312 added the *out-of-scope Posted issue is not in scope of VS Code label Dec 4, 2023
@vscodenpa
Copy link

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!

@vscodenpa vscodenpa closed this as not planned Won't fix, can't repro, duplicate, stale Dec 4, 2023
@connor4312
Copy link
Member

Closing for housekeeping. I think our current state of things with the JAvaScript Debug Terminal is pretty good. While setting auto attach as a default would be cool for discoverability, I think doing this would cause dramatic feedback from users who start having their editor "do stuff" when they don't want it to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

No branches or pull requests

7 participants