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

CDP Proxy: Allows Other Extensions to Reuse CDP Connection #964

Merged
merged 4 commits into from
Apr 7, 2021
Merged

CDP Proxy: Allows Other Extensions to Reuse CDP Connection #964

merged 4 commits into from
Apr 7, 2021

Conversation

swissmanu
Copy link
Contributor

Extends js-debug to expose its internal CDP connection to other extensions.

This pull request resolves #893.

Overview

  • A new command requestCDPProxy returns websocket connection details to connect to the CDP proxy.
    • If requestCDPProxy is called the first time, the proxy is created
    • Otherwise connection details for the previously created proxy are returned
  • The debug adapter holds an instance of the CDP proxy and ensures forwarding of any communication between proxy clients and the CDP connection.
  • The proxy and its clients use a basic JSON communication protocol described here: https://github.com/swissmanu/vscode-js-debug-cdp-proxy-api

@ghost
Copy link

ghost commented Apr 6, 2021

CLA assistant check
All CLA requirements met.

@swissmanu
Copy link
Contributor Author

Would it make sense to expose the underlying error here https://github.com/microsoft/vscode-js-debug/blob/main/src/cdp/connection.ts#L271, instead of masking it with undefined? This way, users of the CDP proxy could get a concise error message as well.

@connor4312
Copy link
Member

Yea, the error should be exposed. I'll play around with it. I would prefer to throw/reject with errors, but we depend on omitting the error in so many places throughout js-debug that changing this would be quite risky.

@connor4312
Copy link
Member

@swissmanu can you give me permission to push to your fork please? 🙂

@connor4312
Copy link
Member

connor4312 commented Apr 6, 2021

In the meantime I've pushed my changes onto a branch in 0c6bd3f. As referenced in the commit, I moved the protocol to CDP with a js-debug extension. This simplifies things but it might still be worth publishing a typings package for the js-debug namespace -- right now this is just the single subscribe method but might become more in the future.

Also, now that it's 'just CDP', maybe I should finally look at making js-debug's CDP transports and mechanisms their own package...

@swissmanu
Copy link
Contributor Author

@swissmanu can you give me permission to push to your fork please? 🙂

I ticked the "Allow edits by maintainers" checkbox on this pull request only. Permissions to the fork should be work by now. Sorry for the delay.

I moved the protocol to CDP with a js-debug extension.

I like like your removal of the additonal protocol layer. Makes things straight forward.

@connor4312 connor4312 merged commit e77f62f into microsoft:main Apr 7, 2021
@connor4312
Copy link
Member

🚀 Thank you for your work on this!

@connor4312 connor4312 added this to the April 2021 milestone May 3, 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.

Expose (some) CDP commands
2 participants