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

Detect Blazor WASM projects and add/update appropriate config to allow launching and debugging #3593

Merged
merged 8 commits into from
Feb 26, 2020

Conversation

NTaylorMullen
Copy link
Contributor

@NTaylorMullen NTaylorMullen commented Feb 20, 2020

  • Didn't want to pollute the omnisharp-roslyn language server with an understanding of Blazor WASM projects so built a heuristic on the client to decide if a project looks like a Blazor WASM project. This approach is similar to how web projects are detected today (look at the csproj and see if there's the line sdk=microsoft.net.sdk.web case insensitively). The heuristic for Blazor web assembly project requires the following from the project:

    1. User has the JavaScript Debugger (Nightly) extension installed
    2. User has the debug.node.useV3 setting set to true
    3. User has the debug.chrome.useV3 setting set to true
    4. Contains a launchSettings.json file containing the text "inspectUri".
    5. (Blazor hosted only) Is executable
    6. (Blazor hosted only) References the web SDK
    7. (Blazor hosted only) Netcoreapp targetting
  • Moved some core utility code such as isWebProject into the util.ts file.

  • Created a new ProgramLaunchType enum to influence how launch.json's are created. There's a special case in this new launch enum that knows about Blazor web assembly project types.

  • Added the understanding of the new ProgramLaunchType.BlazorWebAssemblyStandalone and ProgramLaunchType.BlazorWebAssemblyHosted launch types to produce two launch.json configurations:

  1. Launch the Blazor dev server
  2. Launch the Blazor web assembly project in chrome while attached.
  • Added asset tests to verify that we properly generate the new Blazor web assembly configurations.

Example launch.json output for a hosted application:

{
   // Use IntelliSense to find out which attributes exist for C# debugging
   // Use hover for the description of the existing attributes
   // For further information visit https://github.com/OmniSharp/omnisharp-vscode/blob/master/debugger-launchjson.md
   "version": "0.2.0",
   "configurations": [
        {
            "name": ".NET Core Launch (Blazor Hosted)",
            "type": "coreclr",
            "request": "launch",
            // If you have changed target frameworks, make sure to update the program path.
            "program": "${workspaceFolder}/Server/bin/Debug/netcoreapp3.1/BlazorWasmHosted1.Server.dll",
            "args": [],
            "cwd": "${workspaceFolder}/Server",
            "stopAtEntry": false,
            "env": {
                "ASPNETCORE_ENVIRONMENT": "Development"
            },
            "preLaunchTask": "build"
        },
        {
            "name": ".NET Core Debug Blazor Web Assembly in Chrome",
            "type": "pwa-chrome",
            "request": "launch",
            "timeout": 30000,
            // If you have changed the default port / launch URL make sure to update the expectation below
            "url": "http://localhost:5000",
            "webRoot": "${workspaceFolder}/Server",
            "inspectUri": "{wsProtocol}://{url.hostname}:{url.port}/_framework/debug/ws-proxy?browser={browserInspectUri}"
        }
    ]
}

dotnet/aspnetcore#17549

/cc @pranavkm @SteveSandersonMS @mkArtakMSFT @rynowak @javiercn @danroth27

- Didn't want to pollute the omnisharp-roslyn language server with an understanding of Blazor WASM projects so built a heuristic on the client to decide if a project looks like a Blazor WASM project. This approach is similar to how web projects are detected today (look at the csproj and see if there's the line `sdk=microsoft.net.sdk.web` case insensitively). The heuristic for Blazor web assembly project requires the following from the project:
1. Executable
1. A web project (has a csproj that uses the microsoft.net.sdk.web).
1. Targets netstandard2.1 or higher
1. Contains .razor files

- Moved some core utility code such as `isWebProject` into the `util.ts` file.
- Created a new `ProgramLaunchType` enum to influence how launch.json's are created. There's a special case in this new launch enum that knows about Blazor web assembly.
- Added the understanding of the new `ProgramLaunchType.BlazorWebAssembly` to produce two `launch.json` configurations:
1. Launch the Blazor dev server
1. Launch the Blazor web assembly project in chrome while attached.

- Added asset tests to verify that we properly generate the new Blazor web assembly configurations.

https://github.com/dotnet/aspnetcore#17549
@NTaylorMullen NTaylorMullen changed the title Detect Blazor WASM projects and add/update appropriate config. Detect Blazor WASM projects and add/update appropriate config to allow launching and debugging Feb 20, 2020
"type": "pwa-chrome",
"request": "launch",
// If you have changed the default port / launch URL make sure to update the expectation below
"url": "https://localhost:5001",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as the https variant given I assume we want that as the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Technically it would be possible to infer the HTTPS port from launchSettings.json but 5001 is indeed the default.

Inferring it doesn't make a huge difference because (1) the default is already correct, and (2) for a developer who's in the habit of changing it, once they've generated launch.json there would be nothing keeping it in sync with subsequent changes in launchSettings.json.

So basically I think this is fine!

@NTaylorMullen
Copy link
Contributor Author

Note debugger folks: I wasn't able to actually hit breakpoints (but could run and attach) with the private debugger builds due to bugs so please verify my assumptions are correct in the launch.json pieces.

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #3593 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3593   +/-   ##
======================================
  Coverage    89.8%   89.8%           
======================================
  Files          59      59           
  Lines        1589    1589           
  Branches       89      89           
======================================
  Hits         1427    1427           
  Misses        151     151           
  Partials       11      11
Flag Coverage Δ
#integration 100% <ø> (ø) ⬆️
#unit 89.8% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93a1ba6...17fdf62. Read the comment docs.

@filipw
Copy link
Contributor

filipw commented Feb 20, 2020

we already sniff out the Unity projects on the server side in a somewhat similar situation https://github.com/OmniSharp/omnisharp-roslyn/blob/51cf29ca5785ad4973ad0579ece445e7d1992484/src/OmniSharp.MSBuild/Models/MSBuildProjectInfo.cs#L23

I think Blazor WASM could be handled in a similar way, it would be more consistent?

@filipw
Copy link
Contributor

filipw commented Feb 20, 2020

actually, it probably doesn't matter since it can be done entirely on the client side, there is probably no point 👍
we can move it over once it becomes more complex e.g. needs to rely on some msbuild task

@NTaylorMullen
Copy link
Contributor Author

@JoeRobich should I be concerned about the codecov/patch PR check?

@JoeRobich
Copy link
Member

JoeRobich commented Feb 21, 2020

@NTaylorMullen I don't think so. Their detail page even says everything is the same. Feel free to merge
image

@SteveSandersonMS
Copy link
Member

a heuristic on the client to decide if a project looks like a Blazor WASM project

This is great for the "standalone" Blazor WebAssembly case, but is there anything here to account for the "Blazor WebAssembly hosted in ASP.NET Core" case? This is equally important.

If you want a detailed heuristic for this, it could be something like "an ASP.NET Core project that references another project that matches the heuristic for Blazor WebAssembly". A simpler alternative would just be: "for all ASP.NET Core projects, offer the option to add the '"Launch Blazor web assembly project in Chrome"' configuration".

Note that in the "hosted" case, it's not necessary to add the configuration for launching the Blazor WebAssembly dev server. Instead, we only need the "Launch Blazor web assembly project in Chrome" part.

Does VS Code have a way to do simultaneous mixed-mode debugging (.NET and the JS debugger which we use for Mono WebAssembly)? If so that's optimal for this case, as the developer might want to have breakpoints in both their server-side and client-side .NET code. This is how it works in VS. If there isn't a way to do simultaneous mixed-mode debugging then I guess the scenario doesn't apply.

@NTaylorMullen
Copy link
Contributor Author

NTaylorMullen commented Feb 24, 2020

Does VS Code have a way to do simultaneous mixed-mode debugging (.NET and the JS debugger which we use for Mono WebAssembly)?

Yup, they just launch multiple configurations simultaneously.

is there anything here to account for the "Blazor WebAssembly hosted in ASP.NET Core" case? This is equally important.

Nope, nothing here to account for that. In that case the project will be understood as a classic ASP.NET web project. I need to see what it takes to add a configuration option to launch and debug a blazor web assembly project in chrome.


With help from others I found a way to utilize compound launch configuration to potentially launch and debug a Blazor WASM project in one go; seeing if it fulfills all cases. Assuming it does, going to work on updating this PR to use that.

- Changed how we detect a Blazor web assembly project. The new criteria consists of reading a projects launchSettings.json file and if it has the text `"inspectUri"` to treat it as a Blazor WASM project. This is similar to how VS detects Blazor WASM.
- Added a new `ProgramLaunchType` of `BlazorWebAssemblyHosted` and factored the existing `BlazorWebAssembly` launch type to be `BlazorWebAssemblyStandalone`.
- `BlazorWebAssemblyHosted` is based on the following criteria:
  1. A Blazor web assembly project (`launchSettings.json` containing `"inspectUri"`).
  2. Executable
  3. References the web SDK
  4. Netcoreapp targetting
- Added new tests for the Blazor web assembly hosted project types.
@NTaylorMullen
Copy link
Contributor Author

🆙 📅

Add understanding of Blazor hosted applications.

  • Changed how we detect a Blazor web assembly project. The new criteria consists of reading a projects launchSettings.json file and if it has the text "inspectUri" to treat it as a Blazor WASM project. This is similar to how VS detects Blazor WASM.
  • Added a new ProgramLaunchType of BlazorWebAssemblyHosted and factored the existing BlazorWebAssembly launch type to be BlazorWebAssemblyStandalone.
  • BlazorWebAssemblyHosted is based on the following criteria:
    1. A Blazor web assembly project (launchSettings.json containing "inspectUri").
    2. Executable
    3. References the web SDK
    4. Netcoreapp targetting
  • Added new tests for the Blazor web assembly hosted project types.

return false;
}

if (!project.IsExe) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why is this check important? I'd think the latter two checks would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just following suite with how O# typically checks "is runnable web" project.

@JoeRobich
Copy link
Member

@NTaylorMullen Is this ready to merge?

@NTaylorMullen
Copy link
Contributor Author

@NTaylorMullen Is this ready to merge?

No not quite. We're still trying to figure out timelines and approaches.

- Added a restriction to our Blazor detection/asset prefilling tech. The new restriction is:
  1. User must have the `JavaScript Debugger (Nightly)` extension.
  2. User must have the `debug.node.useV3` flag set to `true`
  3. User must have the `debug.chrome.useV3` flag set to `true`

- Removed the launch.json compounds due to timing issues in core VSCode. The new experience now relies on a user hitting "launch Standalone/hosted" and then doing "debug Blazor xyz". As part of this I removed the `priority`presentation order since we no longer need to force ordering with a compound.
@NTaylorMullen
Copy link
Contributor Author

NTaylorMullen commented Feb 26, 2020

🆙 📅

Restrict Blazor WASM asset additions

  • Added a restriction to our Blazor detection/asset prefilling tech. The new restriction is:

    1. User must have the JavaScript Debugger (Nightly) extension.
    2. User must have the debug.node.useV3 flag set to true
    3. User must have the debug.chrome.useV3 flag set to true
  • Removed the launch.json compounds due to timing issues in core VSCode. The new experience now relies on a user hitting "launch Standalone/hosted" and then doing "debug Blazor xyz". As part of this I removed the prioritypresentation order since we no longer need to force ordering with a compound.

  • Add prompt to users to indicate Blazor WASM debuggig is not configured properly for their VSCode.
    image

/cc @SteveSandersonMS @danroth27 @mkArtakMSFT

src/omnisharp/utils.ts Outdated Show resolved Hide resolved
@@ -382,43 +354,10 @@ export function createBlazorWebAssemblyDevServerLaunchConfiguration(workingDirec
"type": "coreclr",
"request": "launch",
"program": "dotnet",
"args": ["run", "--no-build"],
"args": ["run"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good change. I was a bit concerned about people getting tripped up by --no-build before.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Feb 26, 2020

Added a restriction to our Blazor detection/asset prefilling tech. The new restriction is:
User must have the JavaScript Debugger (Nightly) extension.
User must have the debug.node.useV3 flag set to true
User must have the debug.chrome.useV3 flag set to true

What happens longer term? I'm guessing the V3 debugger becomes the default at some point, and then people will no longer have the nightly extension nor these config flags.

What's the best way for us to (a) know when that change is going to roll into the mainline product, so we can remove this logic in the same release, or (b) better still, set up the logic so that it automatically works once the V3 debugger is the default?

@SteveSandersonMS
Copy link
Member

@NTaylorMullen This looks super to me. I don't get the option to leave a proper sign-off but FWIW I would do.

I would still like to have your thoughts on how we should best react when the V3 debugger becomes default, and if there's any reasonable way to deal with it in advance. But besides that I think we're all good here!

Co-Authored-By: Steve Sanderson <[email protected]>
@NTaylorMullen
Copy link
Contributor Author

I would still like to have your thoughts on how we should best react when the V3 debugger becomes default, and if there's any reasonable way to deal with it in advance. But besides that I think we're all good here!

So we'll have to pre-emptively make a change and release to O# to account for this where we'll add an additional condition where if the release extension (javascript-debugger I assume) is of a certain version or higher to then skip our nightly extension checks.

As for how we'll know when they're about to RTM; hopefully they'll be kind enough to let us know a date once one is confirmed at which point we can schedule the work on our end.

@NTaylorMullen NTaylorMullen merged commit b199d38 into master Feb 26, 2020
@NTaylorMullen NTaylorMullen deleted the nimullen/17549 branch February 26, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants