Skip to content

Conversation

alt-romes
Copy link
Collaborator

Fixes #15

@alt-romes
Copy link
Collaborator Author

@dmjio I'm using this change in the debugger successfully. Perhaps you'd like to review?

Copy link
Contributor

@dmjio dmjio left a comment

Choose a reason for hiding this comment

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

Looks good, but wondering if we can reuse send here.

Exposes 'sendRunInTerminalReverseRequest' helper too.
@alt-romes alt-romes force-pushed the wip/romes/things-for-0.3 branch from 5ec87b6 to 9457d40 Compare October 2, 2025 09:16
@alt-romes
Copy link
Collaborator Author

@dmjio I've simplified according to your suggestions.

I was also wondering about serviceClient and getRequest. I was trying to extend the communicate higher order function to receive Either ReverseCommand Command, to deal with reverse request responses.

(Currently, the code assumes the server will only ever send requests, but with reverse requests it will also send reverse request responses)

The issue is the request type argument to Adaptor-- why is it needed? request is instanced to Request in functions such as send and in calls such as getArguments. However, I was coming up with a new type for ReverseRequestResponse, which now doesn't fit in the Adaptor.


Why have the request type argument? Is it not always Request in practice? I was thinking briefly about a refactor where we could get rid of the request field in the AdaptorLocal reader environment. Instead, the higher order function passed to serviceClient receive a Either ReverseRequestResponse Request instead of Either ReverseCommand Command (where Command is an enum without fields and Request is a struct containing the Command enum plus the arguments which are currently fetched from the reader using getArguments).

Let me try this quickly...

@alt-romes
Copy link
Collaborator Author

Why have the request type argument? Is it not always Request in practice?

Ah, I see the point. We only have a Request in the environment when replying to a request. Notably, we don't have one in registerNewDebugSession but we do have one in communicate.

Extends the entrypoint runDAPWithLogger function to accept a function
which is called on `ReverseRequestResponse`s, a response to a reverse
request. This allows the DAP server to e.g. capture the PID of the
process invoked via `runInTerminal`.
@alt-romes
Copy link
Collaborator Author

@dmjio I've added a third commit to allow receiving reverse-request-responses. Let me know what you think.

@alt-romes
Copy link
Collaborator Author

alt-romes commented Oct 3, 2025

@dmjio I've also updated the CHANGELOG and bumped the version hoping we could do another 0.x.0.0 release with the added capability.

@dmjio
Copy link
Contributor

dmjio commented Oct 6, 2025

I think ReverseRequestResponse in getRequest is reasonable. This is the first time we're adding support for the reverse flow, so maybe there is a more idiomatic way of doing things, but this seems fine to me.

@dmjio dmjio self-requested a review October 6, 2025 19:17
@alt-romes alt-romes merged commit 41558e8 into master Oct 7, 2025
1 check passed
@dmjio dmjio deleted the wip/romes/things-for-0.3 branch October 7, 2025 22:00
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.

'runInTerminal' not supported by API

2 participants