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

Use System.Text.Json for serialization #10551

Merged
merged 26 commits into from
Jul 3, 2024
Merged

Use System.Text.Json for serialization #10551

merged 26 commits into from
Jul 3, 2024

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Jun 27, 2024

Fixes #10539

This PR switches us to use System.Text.Json for server to client communication, and opts us in to using it for client to server communication too.

Needs https://devdiv.visualstudio.com/DevDiv/_git/WebTools/pullrequest/561188 to flow to VS main, then I'll kick off a final integration test run for good measure. Without that PR the Wrap With Tag functionality doesn't work.

TODO:

  • Test VS Code

@davidwengier davidwengier requested a review from a team as a code owner June 27, 2024 07:30
@davidwengier
Copy link
Contributor Author

VS Code issues:

@davidwengier
Copy link
Contributor Author

davidwengier commented Jul 1, 2024

Integration tests: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=9811665&view=results ✅ Passed

Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/561892

  • RPS ✅ Passed
  • Speedometer ❓Razor tests passed, but had some issues. Seems there is a known outage for them being somewhat unreliable.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Excellent change. Well done!

@@ -101,7 +101,7 @@ public DefaultCSharpCodeActionProvider(LanguageServerFeatureOptions languageServ
// flag is on, any perf hit here isn't going to affect real users.
try
{
if (((JToken)codeAction.Data).ToObject<RazorCodeActionResolutionParams>() is not null)
if (((JsonElement)codeAction.Data).Deserialize<RazorCodeActionResolutionParams>() is not null)
Copy link
Member

Choose a reason for hiding this comment

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

This construct is a little strange. I did a bit of a double take when I saw it. 😄

Consider adding a helper to clarify this. Could that be used elsewhere? Maybe something like this?

public static bool CanDeserializeTo<T>(this JsonElement jsonElement)
    => jsonElement.Deserialize<T> is not null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to do that in a local function, but I don't really want to put it on a help class anywhere, because I would hate for anyone to think that its a useful function to call. This line is only hit when a feature flag is on, that is only for our information, and only possible to turn on for MS internal users, so the badness of the code doesn't worry me here.

@davidwengier
Copy link
Contributor Author

Tests are broken because someone decided to update our DocumentSymbol support, so I now I need new VS protocol packages, which means I'm in "Detected package downgrade" hell. Will merge once I escape.

# Conflicts:
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/LegacyRazorCompletionEndpointTest.cs
#	src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/LegacyRazorCompletionResolveEndpointTest.cs
@davidwengier davidwengier requested a review from a team as a code owner July 3, 2024 01:42
@davidwengier davidwengier merged commit 50bc118 into main Jul 3, 2024
12 checks passed
@davidwengier davidwengier deleted the dev/dawengie/STJ branch July 3, 2024 04:02
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 3, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024
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.

Move to newer LSP APIs (System.Text.Json)
4 participants