-
Notifications
You must be signed in to change notification settings - Fork 727
Implement debug proxy to display error message before install completes #135
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
Conversation
| }); | ||
|
|
||
| return promise; | ||
| /*--------------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this file is showing as one big delete and add. possibly because of the rename? I thought github could handle this better. Maybe I'm missing something? Doesn't seem to be a line ending thing.
|
LGTM |
src/coreclr-debug/util.ts
Outdated
| let _installBeginFilePath: string = ''; | ||
| let _installCompleteFilePath: string = ''; | ||
|
|
||
| export function setExtensionDir(extensionDir: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... a bunch of state and function that operate on it. Maybe just make a class? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably a good call. still not super used to javascript/typescript.
|
@DustinCampbell I abstracted util into a class. Is this more what you had in mind? |
| _channel.show(vscode.ViewColumn.Three); | ||
| } | ||
| }); | ||
| statusBarMessage.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this in the success case too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking if we want to dispose the first message before setting the new success message? Probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it's written, the success message will never be disposed. The only thing we could do is set it to be disposed after a certain timeout.
|
Otherwise looks good to me. |
This adds a debug proxy that acts as the debug adapter program before the coreclr-debug are fully downloaded. If the user tries to start debugging before download is finished, the proxy displays a sane error message. Once the download is complete, we rewrite the manifest to no longer call the proxy. During the VS Code session that downloads the components, the manifest is rewritten but not reloaded. In this case the proxy spawns the real debugger as a child process and proxies its stdin/stdout. Once vs code is restarted the new manifest is loaded and the proxy is no longer called. Also adds an empty command that can be run to force activation of the C# extension, which will kick off debugger acquisition.
This adds a debug proxy that acts as the debug adapter program before the
coreclr-debug are fully downloaded. If the user tries to start debugging
before download is finished, the proxy displays a sane error message. Once
the download is complete, we rewrite the manifest to no longer call the
proxy.
During the VS Code session that downloads the components, the manifest is
rewritten but not reloaded. In this case the proxy spawns the real
debugger as a child process and proxies its stdin/stdout. Once vs code is
restarted the new manifest is loaded and the proxy is no longer called.
Also adds an empty command that can be run to force activation of the C#
extension, which will kick off debugger acquisition.