-
Notifications
You must be signed in to change notification settings - Fork 687
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
Delay server start if nothing opened to avoid blocking devkit #8026
base: main
Are you sure you want to change the base?
Conversation
@@ -83,6 +83,8 @@ export async function activate( | |||
const installDependencies: IInstallDependencies = async (dependencies: AbsolutePathPackage[]) => | |||
downloadAndInstallPackages(dependencies, networkSettingsProvider, eventStream, isValidDownload); | |||
|
|||
// Fine to do this as part of activate as it is always a no-op in Roslyn mode (the debugger and Razor are shipped in-box) | |||
// Only actually downloads files in O# mode or local development. | |||
const runtimeDependenciesExist = await installRuntimeDependencies( |
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.
debugger and razor are shipped in box now. I suspect I can actually have this only run in O# mode, but I'll need to make sure debugger/razor are installed in the specific local development scripts. for a followup.
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.
Suggestion: Since I believe we still need to upload a platform-neutral .vsix, but nearly no one should be using it, could we use conditional compilation so that the platform neutral one still used this code, but the platform-specific one omitted the check except in O# mode.
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.
I don't know of a way to do conditional compilation in TS easily (but I'm no TS expert). The only issue I found about was closed as not planned - microsoft/TypeScript#3538
You're right that the neutral build here would also be a problem, I had forgotten about that one. I don't think we actually even upload a platform neutral build to the marketplace anymore - perhaps we should consider not even building it.
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.
One option would be to have a isInPlatformSpecificVSIX
function with two implementations, and we change the compiler options to include the one we want.
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.
Or maybe easier: we could add something to package.json
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.
Or maybe easier: we could add something to package.json
Didn't quite get this - you mean add some kind of mapping of platform -> packages to download in the package.json?
As an aside - was planning on tackling this particular issue in a followup anyway (install no-ops in the cases we care about already), wanted to check if you had any other issues with the idea and approach in this PR from the debugger side. I know there are activation events for debug events, but I do not know if they are applicable w/out a workspace or file @gregg-miskelly
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.
wanted to check if you had any other issues with the idea and approach in this PR from the debugger side.
I don't have any problems with merging from the debugger side. I think the only possible issue from the debugger side would be if something would now fail, such as attempts to launch a project, because the language service is no longer running. But, as far as I know, we already have those bugs today because we already don't block waiting for activation. CC @WardenGnaw in case there is something I am not thinking of.
Didn't quite get this - you mean add some kind of mapping of platform -> packages to download in the package.json?
My thought was we could add a new property to package.json that we would twiddle as part of the build, and since we have access to the package.json content as part of activation, we could check this. Example: if (!context.extension.packageJSON.platformSpecificVSIX || useOmnisharpServer)
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.
Discussed offline, these changes will affect the vscode commands that are defined in package.json
but not yet registered yet.
back to draft - need to ensure that everything starts in response to certain commands (e.g. attach to process) even in no workspace scenarios. |
Previously, we have attempted to be good citizens of activation by only activating on specific relevant events, and not blocking activation on the server process starting.
However - when Dev Kit is causing us to activate in order to provide the new project command, we end up taking extension host main thread resources away anyway because we launch the server after extension activation. This delays the dialog from showing up.
Typically, in the new project scenario we cannot provide anything useful anyway - most often there is no workspace and no open files. The request is on us to avoid as much work as possible until we detect something actionable.
The change in this PR delays server startup until a workspace is opened or a c# file exists.