Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13930Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13930" |
@dotnet-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR refactors the serverReadyAction handling in the VS Code extension to honor user-specified server ready actions from the Aspire launch configuration, enabling browser debugger attachment (e.g., debugWithChrome, debugWithEdge) and custom URL patterns. Previously, a hard-coded default configuration would overwrite any user-supplied settings.
Changes:
- Added
ServerReadyActiontype definition and exposed it onAspireExtendedDebugConfiguration - Refactored
determineServerReadyActionto accept options and inherit from parent debug configuration - Updated pattern to use capture groups (
(https?://\\S+)) for proper URL substitution with%sformat
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/src/dcp/types.ts | Added ServerReadyAction type and parentDebugConfiguration field to LaunchOptions and AspireExtendedDebugConfiguration |
| extension/src/debugger/launchProfiles.ts | Refactored determineServerReadyAction to accept options object and support parent configuration inheritance |
| extension/src/debugger/languages/dotnet.ts | Updated call to determineServerReadyAction to pass parentDebugConfiguration from launchOptions |
| extension/src/test/launchProfiles.test.ts | Updated tests to use new API and added cross-platform path handling |
| extension/src/test/dotnetDebugger.test.ts | Updated assertions to match new serverReadyAction behavior with capture groups |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| export function determineServerReadyAction( | ||
| options: ServerReadyActionOptions = {} | ||
| ): ServerReadyAction | undefined { | ||
| if (!options.launchBrowser) { |
There was a problem hiding this comment.
Should we actually gate this on launchSettings.json's launchBrowser property?
launchSettings.json (IMO) is a relic from Visual Studio
launch.json (IMO) is the "VS Code" way
I know that's an over-simplification and that we are conceptually layering them so launch.json -> launchSettings.json -> target project
I guess my point is it took me a while to realize that both applicationUrl and launchBrowser both needed to be specified before I could have the privilege of using the hardcoded serverReadyAction.
I don't want other VS Code purists to get frustrated that their serverReadyAction isn't working because a setting for Visual Studio isn't specified.
There was a problem hiding this comment.
It seems reasonable to not need launchBrowser specified if there is not a launchSettings.json input. I'll make the change
There was a problem hiding this comment.
You know, I thought about this some more and it occurred to me that in the absence of launchBrowser: true, we may end up attaching a serverReadyAction to multiple projects that don't need it. My understanding is that the serverReadyAction uses an expensive call to vscode.window.onDidWriteTerminalData so we might actually want to keep this and document it.
There was a problem hiding this comment.
I like the idea that specifying serverReadyAction on the entire debug session will apply to all started resources in the application.
We are going to expose a hook to apply debugger properties to specific resources, if that's the behavior you're looking for - #12711
extension/src/dcp/types.ts
Outdated
| debugSessionId: string; | ||
| isApphost: boolean; | ||
| debugSession: AspireDebugSession; | ||
| parentDebugConfiguration?: AspireExtendedDebugConfiguration; |
There was a problem hiding this comment.
Strangely, this added line in the diff does not show up in the actual branch code.
| ): LaunchProfileResult { | ||
| const debugMessage = | ||
| `[launchProfile] determineBaseLaunchProfile: | ||
| disable_launch_profile=${launchConfig.disable_launch_profile === true} |
There was a problem hiding this comment.
nit: !! instead of true equality check
| export function determineServerReadyAction( | ||
| options: ServerReadyActionOptions = {} | ||
| ): ServerReadyAction | undefined { | ||
| if (!options.launchBrowser) { |
There was a problem hiding this comment.
It seems reasonable to not need launchBrowser specified if there is not a launchSettings.json input. I'll make the change
|
Ok, I've wrapped my head around the problem here. The solution presented seems incomplete (ie, Short term, to provide a workaround, if it is then we should override any existing server ready action. Longer term, overlaps with #12711 as the explicit launch.json |
|
@dkattan how do my changes look to you? |
|
I've been thinking more about this, and would like to move these changes into #12711, closing this PR for now. |
|
I'm reopening this because the changes here work with and don't seem to overlap with #12711 and it would at least unstick me and allow debugging without waiting for that other PR @adamint @davidfowl |
Motivation
Aspire 13 improved debugging for child processes; this change extends that all the way to the browser.
Expectation
If I use the following configuration in
launch.json{ "name": "Aspire", "type": "aspire", "request": "launch", "program": "${workspaceFolder}/Aspire/Immybot.AppHost/Immybot.AppHost.csproj", "serverReadyAction": { "action": "debugWithEdge", "pattern": "\\bNow listening on:\\s+(https?://\\S+)", "killOnServerStop": true, } }Then Edge will start with the debugger attached and navigate to http://localhost:50061 when my application emits
Now listening on: http://localhost:50061Actual behavior
launchSettings.jsondid not haveapplicationUrlsetapplicationUrldoesn't support tokens/interpolation so I can't have it automatically launch to the port DCP chose for the frontendRoot causes
serverReadyActionapplicationUrlfromlaunchSettings.jsonHard-coded
serverReadyActionUser supplied
serverReadyActionon the Aspire launch config is being overwritten by an over-simplistic hard-coded default inlaunchProfiles.tsThat prevented browser breakpoint debugging and custom URL capture patterns. In this PR we honor the supplied
serverReadyAction, so browser debugging can be triggered correctly (e.g.,debugWithChrome/debugWithEdge) with a custom pattern and uriFormat.Dependence on
applicationUrlfromlaunchSettings.jsonIn my scenario, Aspire is dynamically assigning ports to the frontend (so I can have multiple worktrees going for maximum vibing) but
applicationUrlfromlaunchSettings.jsonis not expanded to support any sort of tokens/interpolation from env vars and must have valid URI syntax (aside from splitting on;).We could simply adjust
patternto actually capture the emitted URL and useuriFormat: "%s"instead, which would be better but you'd still be hardcoding other properties likeaction: "openExternally"and not honoring the user's specifications.Changes
ServerReadyActionto DCP types and expose it onAspireExtendedDebugConfiguration.ServerReadyActionand link to its canonical definition.determineServerReadyAction.parentDebugConfiguration(the user's supplied launch configuration) intodetermineServerReadyActionfrom the .NET debugger and use it if available and only fall-back to the existing hard-coded one if it doesn't exist.Tests
launchProfiles.test.tsdotnetDebugger.test.ts