Skip to content

Add cohost option to Razor #8189

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

Merged
merged 4 commits into from
Apr 22, 2025
Merged

Conversation

davidwengier
Copy link
Member

@davidwengier davidwengier commented Apr 16, 2025

Goes with dotnet/roslyn#78167 and dotnet/razor#11748 to enable the start of Razor cohosting in VS Code.

Mainly just adding a new option, and using it to turn off a bunch of things, but also a slight tweak to configuration names which I'll call out.

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things we need to think about eventually

  1. Do we want to group Razor server logs wit hthe C# logs? Or should we split them out instead?
  2. Razor report issue - does the command need to be updated to pull different logs / describe the cohosting option value?
  3. For VSCode, the experience is using the same language server instance as C# right? Just with language specific handlers and dynamic registration of those features?
  4. There's a lot of random feature related code in the client side, e.g. https://github.com/dotnet/vscode-csharp/blob/main/src/razor/src/completion/completionHandler.ts - how much of this applies to cohosting?

@@ -430,6 +431,9 @@ class RazorOptionsImpl implements RazorOptions {
public get razorServerPath() {
return readOption<string>('razor.languageServer.directory', '');
}
public get cohostingEnabled() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this option requires a reload? If so we should add it to the needs reload options

Copy link
Member Author

@davidwengier davidwengier Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all Razor options currently need a reload, and none of them are configured to prompt for reload :)

@davidwengier
Copy link
Member Author

davidwengier commented Apr 16, 2025

A couple things we need to think about eventually

Very good questions! This is definitely just a start on cohosting, and dotnet/razor#11742 is only adding semantic tokens support specifically because it allows us to punt on some of these things and do them in separate PRs.

  1. Do we want to group Razor server logs wit hthe C# logs? Or should we split them out instead?

Logs from Razor currently come from here and it would be pretty trivial to use a custom message and put them back in the Razor output window.

  1. Razor report issue - does the command need to be updated to pull different logs / describe the cohosting option value?

Probably.

  1. For VSCode, the experience is using the same language server instance as C# right? Just with language specific handlers and dynamic registration of those features?

Yes. No rzls, no razorLanguageServerClient.

  1. There's a lot of random feature related code in the client side, e.g. main/src/razor/src/completion/completionHandler.ts - how much of this applies to cohosting?

The bits that apply are the ones that maintain Html virtual documents, and forward requests on to them. That's why I started with semantic tokens, because it doesn't. Those endpoints will have to move though, to the roslyn server, and some of our services will either need to be able to take both roslyn and razor servers as parameters, or more likely we can use a common interface. There is more work on the Razor side before that is needed though.

razorComponentPath = path.dirname(extPath);
});

// If cohosting is enabled we get the source generator from the razor component path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't they be the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a real build, yes they'd be the same, but for local development not necessarily. If you're only working on cohosting, then there is no need to specify a local path for rzls at all, since it won't run. Plus this makes more sense in future when the .razor folder will presumably go away entirely.

@davidwengier davidwengier merged commit 2db499b into dotnet:main Apr 22, 2025
24 checks passed
@davidwengier davidwengier deleted the CohostVSCode branch April 22, 2025 00:38
davidwengier added a commit to dotnet/roslyn that referenced this pull request Apr 22, 2025
Goes with dotnet/vscode-csharp#8189 and
dotnet/razor#11748 to allow Razor cohosting in
VS Code, but also fixes some VSIX issues that broke cohosting in VS too.
Reviewing commit-at-a-time is probably recommended.
davidwengier added a commit to dotnet/razor that referenced this pull request Apr 24, 2025
Goes with dotnet/roslyn#78167 and
dotnet/vscode-csharp#8189 to expose cohosting to
VS Code.

Needs dotnet/roslyn#78167 merged and referenced
before it will build.
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.

3 participants