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

Delay server start if nothing opened to avoid blocking devkit #8026

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Feb 27, 2025

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.

@dibarbet dibarbet requested a review from a team as a code owner February 27, 2025 02:02
@@ -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(
Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

@dibarbet dibarbet Feb 27, 2025

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Member Author

@dibarbet dibarbet Feb 27, 2025

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

Copy link
Contributor

@gregg-miskelly gregg-miskelly Feb 27, 2025

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)

Copy link
Contributor

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.

@dibarbet dibarbet marked this pull request as draft February 28, 2025 00:32
@dibarbet
Copy link
Member Author

back to draft - need to ensure that everything starts in response to certain commands (e.g. attach to process) even in no workspace scenarios.

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.

4 participants