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

feat: allow custom request handlers #281

Closed

Conversation

entropitor
Copy link
Contributor

Based off #136 (comment) I thought to give it another try for vscode-js-debug and I was able to get it working. The only thing I need is this request. While python uses an event to "announce" the sessions (https://github.com/mfussenegger/nvim-dap-python/pull/21/files), vscode-js-debug uses a request named attachedChildSession

@entropitor
Copy link
Contributor Author

For optimal flow, we should also have a way to run some clean-up code from the function which is called here: https://github.com/mfussenegger/nvim-dap/blob/master/lua/dap.lua#L253-L258 currently we can only return a config through the callback but for the vscode-js-debug adapter, we would need a way to clean-up a process as the user exits the session. There are a couple of options but I would need some guidance on how you would approach it unless it doesn't matter to you?

@mfussenegger
Copy link
Owner

mfussenegger commented Aug 26, 2021

One of the reasons I closed #145 without merging is that the primary difference between events and reverse requests is that reverse requests can (or should) have a response.

The current listener pattern doesn't really fit there - because they can only listen and not provide a response.

So I'm reluctant to merge this. The fact that two Microsoft debug adapters have the same requirement, but ended up implementing it differently, and show little incentive to get this standardized in the protocol specification doesn't really help.

I'm not sure yet how to deal with this. I'll give it some thought.

@mfussenegger
Copy link
Owner

One option could be to give an adapter definition the possibility to define and implement custom reverse request handlers.
I'm not sure if they should be allowed to override the built-in handler - but in any case there would only be single handler per request to make it clear who sends the response

@entropitor
Copy link
Contributor Author

Yeah, potentially we should take a look at how neovim set it up in LSP handlers? Given this feels very similar (with the difference that here, there is the extra notion of a session).

For me the exact format doesn't matter as much but it's clear we need to do something to be able to support the js one and apparently another one.

The weird thing is that I'm not even sending a response and it's still working 😅 So I guess they should have used an event instead of a request but 🤷

@mfussenegger
Copy link
Owner

Yeah, potentially we should take a look at how neovim set it up in LSP handlers? Given this feels very similar (with the difference that here, there is the extra notion of a session).

I'm fairly familiar with how the LSP handlers work in neovim.

To me it is mostly a matter of deciding whether the custom handler should have preference over built-ins or not. Advantage of having preference is that it would be possible to override things like the runInTerminal handling. This would currently kinda work, because runInTerminal is quite separated and the rest of nvim-dap doesn't rely too much on any logic happening there.

The disadvantage is that if the specification is extended and nvim-dap adds built-in handling for a new reverse request, and other nvim-dap functionality depends on that logic being run - plugins or other user overrides could mess with it and break things.

I tend towards not allowing to override built-ins, but merely to provide implementations for handlers that are missing. (This would mean nvim-dap can break plugins once it would add built-in implementations, instead of potentially the other way around)

@entropitor
Copy link
Contributor Author

I'd personally allow others to extend. And if others break the flow, let them know they should call the default handler as well / let them fix it but at least then you have the option to extend it a bit if you want. But I think both could work

@mfussenegger
Copy link
Owner

This #286 shows what I had in mind.

mfussenegger added a commit that referenced this pull request Aug 31, 2021
@entropitor
Copy link
Contributor Author

Closing in favor of #286

@entropitor entropitor closed this Aug 31, 2021
@entropitor entropitor deleted the allow-request-handler branch August 31, 2021 20:57
@entropitor entropitor restored the allow-request-handler branch September 5, 2021 18:55
mfussenegger added a commit that referenced this pull request Sep 30, 2021
mfussenegger added a commit that referenced this pull request Sep 30, 2021
mfussenegger added a commit that referenced this pull request Sep 30, 2021
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.

2 participants