-
Notifications
You must be signed in to change notification settings - Fork 199
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
Add support for one-step debugging for Blazor #1885
Add support for one-step debugging for Blazor #1885
Conversation
ade6ddb
to
e0bf1f0
Compare
e0bf1f0
to
88e4fad
Compare
@captainsafia @NTaylorMullen Do we also need to update the Blazor WebAssembly auto detect functionality in the C# extension to use this new launch profile? Or will that be handled separately? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/extension.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really awesome to see this all come together! I also totally get what you were saying in regards to testability of this. @gregg-miskelly @connor4312 how do you guys handle doing tests for your debug adapters? Also would you mind taking a peek at this PR?
Lastly, I envision us also wanting some telemetry around how often users are debugging their Blazor WASM apps in VSCode. Assuming that's a good idea (@danroth27 ?) would you mind filing an issue for that?
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/extension.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/extension.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/extension.ts
Outdated
Show resolved
Hide resolved
That has to be done in O# but I can assist @captainsafia in doing that. It wont be too rough given I already did the bulk of that work 😄 |
We wrote a little C# framework that we use for testing the C# and C++ Debug Adapters. But it would be overkill for what you have here. The VS Code team has a test framework that might be helpful: npm, GitHub WAIT: Ignore this comment. I tried to answer this question before my last meeting when I only did a quick skim of the review - since you don't have a real debug adapter, I don't think this would help you test this. |
@@ -15,6 +15,7 @@ | |||
"typescript": "3.3.4000" | |||
}, | |||
"dependencies": { | |||
"ps-list": "^7.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just remembered, you'll also need to make sure this is tracked in component governance as well: https://dev.azure.com/dnceng/internal/_componentGovernance/dotnet-aspnetcore-tooling?_a=alerts&typeId=1981311&alerts-view-option=active
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
The bulk of the tests for js-debug test against the debug logic via DAP and bypass the UI--this is faster and lets us test more behavior more exactly and explicitly versus testing for changes within the UI. But there's also a few tests, like some tests for profiling, where we test end to end through the UI via a call to |
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
8574082
to
528dba9
Compare
528dba9
to
6757fca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
90a7258
to
ce06cf1
Compare
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode.Extension/package.json
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
constructor(private readonly logger: RazorLogger, private readonly vscodeType: typeof vscode) { } | ||
|
||
public async resolveDebugConfiguration(folder: vscode.WorkspaceFolder | undefined, configuration: vscode.DebugConfiguration): Promise<vscode.DebugConfiguration | undefined> { | ||
const shellPath = process.platform === 'win32' ? 'cmd.exe' : 'dotnet'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this in the first pass. @gregg-miskelly do you all do anything special in the case dotnet
is not on the path? Wondering if we should be notifying users if it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For our implementation, the error will be visible in the terminal in the workspace. Not sure if we want to check for dotnet
in the path ahead of time and do something prominent like show an error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the C# extension is installed (I think it always will be?) the user will get a prompt telling them that dotnet could not be located. That said, lots of people ignore errors. So if you are just showing a generic 'file not found' error, it might be helpful to print out a bit extra. You could link to: https://aka.ms/VSCode-CS-DotnetNotFoundHelp
In reply to: 424869410 [](ancestors = 424869410)
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/Events.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/Events.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/Events.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/Events.ts
Outdated
Show resolved
Hide resolved
0f59289
to
3d0c3f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had minor comments but you should probably wait for @gregg-miskelly to confirm this behavior is "ok".
Other then my minor comments, everything is looking really really good! Awesome job @captainsafia! It's nice seeing this all come together and most of all enable the beginnings of our debugging future 😄.
Once this is in let me know if you need some help updating O#'s side to generate the correct pieces, I can point you in the right direction.
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/BlazorDebugConfigurationProvider.ts
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.VSCode/src/BlazorDebug/TerminateDebugHandler.ts
Outdated
Show resolved
Hide resolved
3d0c3f9
to
00f5ee2
Compare
00f5ee2
to
146d0dd
Compare
32dc01d
to
0fd5492
Compare
Thanks for the feedback, all! @gregg-miskelly and @connor4312: anything else you'd like to add? |
* On non-Windows platforms, we need to terminate the Blazor | ||
* dev server and its child processes. | ||
*/ | ||
const terminate = this.vscodeType.debug.onDidTerminateDebugSession(async event => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fire when any debug session finishes -- you want to look at the session to make sure it's the one we care about. (e.g. I might have an API server written in some other language, stopping/restarting that shouldn't cause my blazor debugging to stop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sensible. The type
property on the DebugSession seemed like the best thing to check against but let me know if there is a better approach here.
Thanks, everyone! Merging this now! 🚀 |
|
||
try { | ||
process.kill(targetPid); | ||
processes.map((proc) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processes.map((proc) => { [](start = 4, length = 25)
Rather than using map
I think you want to instead loop through processes
to find targetPid
. Then loop through just the remaining entries in the map to find any child processes. This way you will avoid the process id reuse problem with ppid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this in a future PR.
…apter Add support for one-step debugging for Blazor\n\nCommit migrated from dotnet/razor@2d31bd0
C# extension 1.22.0 is installed but still it says "The debug type is not recognized. Make sure that you have a corresponding debug extension installed and that it is enabled.(1)" when launch.json in the .vscode folder is: { (I have 3 projects: blazor webassembly, .net core server, shared) |
@fabianus76 The sample in the PR description is out-dated and was only used in development mode. The official docs at https://docs.microsoft.com/en-us/aspnet/core/blazor/debug?view=aspnetcore-3.1#visual-studio-code describe how to set up debugging in VS Code. |
@captainsafia - fantastic, thanks for the feedback ! |
@fabianus76 Can you open a new issue in https://github.com/dotnet/aspnetcore for help with this? |
Changes in the PR
In order to use this new debug adapter, the user can add this to their launch configuration.
To launch in Edge instead of Chrome, the user can use the
browser
config option.The user can also pass environment variables with the
env
config option and set the URL to open in the browser with theurl
option.It also supports the underlying
timeout
,webRoot
, andtrace
configuration options.This also fixes the issue where processes lingered on the user's machine when the debug session was stopped on Unix-based systems. Now, a user can run one-step debugging twice in a row on macOS and it should Just Work ™️.
I manually validated this on macOS and Windows.
Addresses dotnet/aspnetcore#19246