-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Switch to STJ for client side LSP serialization #74364
Conversation
public override Task HandleNotificationAsync(string methodName, JsonElement methodParam, Func<JsonElement, Task> sendNotification) | ||
{ | ||
// Razor only ever looks at the method name, so it is safe to pass null for all the Newtonsoft JToken params. | ||
return _razorCSharpInterceptionMiddleLayer.HandleNotificationAsync(methodName, null!, null!); |
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.
named parameters?
@@ -3,19 +3,20 @@ | |||
// See the LICENSE file in the project root for more information. | |||
|
|||
using System; | |||
using System.Text.Json; | |||
using System.Threading.Tasks; | |||
using Microsoft.VisualStudio.LanguageServer.Client; | |||
using Newtonsoft.Json.Linq; | |||
|
|||
namespace Microsoft.CodeAnalysis.Editor.Implementation.LanguageClient; | |||
|
|||
#pragma warning disable CS0618 // Type or member is obsolete - blocked on Razor switching to new APIs for STJ - https://github.com/dotnet/roslyn/issues/73317 |
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.
@@ -267,6 +268,13 @@ public Task OnServerInitializedAsync() | |||
/// </summary> | |||
public Task AttachForCustomMessageAsync(JsonRpc rpc) => Task.CompletedTask; | |||
|
|||
private static PropertyCollection CreateStjPropertyCollection() |
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 vslanguageserverclient code indicates this is a temporary measure for 17.10. Is there going to be a better way to do this soon?
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.
Yes, the client will be opting into STJ by default in 17.12 Preview 1, at which point we can remove this. However we have additional changes, so we need to go in first with an explicit opt-in.
src/Tools/ExternalAccess/Razor/RazorCSharpInterceptionMiddleLayer.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/LanguageServer/AbstractInProcLanguageClient.cs
Outdated
Show resolved
Hide resolved
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.
Resolves #73317
PR val - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/565009