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

Allow services to use System.Text.Json for OOP comms in Razor #74280

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

davidwengier
Copy link
Contributor

Given the amount of inheritance and variation in the LSP types, between VS and VS Code etc. the original plan to wrap LSP types for OOP comms, or add MessagePack compatible attributes, or something, seems way too hard. This is much easier :)

The use of Json is opt-in per service, so this will let us pick and choose the best method on a case-by-case basis.

@davidwengier
Copy link
Contributor Author

@dibarbet @tmat @CyrusNajmabadi PTAL. Mostly Razor EA changes, but one real roslyn change in the remote infra

@davidwengier
Copy link
Contributor Author

Ping @dotnet/roslyn-ide for reviews please

/// </summary>
internal readonly record struct JsonSerializableRazorPinnedSolutionInfoWrapper(
[property: JsonPropertyName("data1")] long Data1,
[property: JsonPropertyName("data2")] long Data2)
Copy link
Member

Choose a reason for hiding this comment

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

interesting. would like to know more about this. when are you guuys pinning solutions?

Copy link
Member

Choose a reason for hiding this comment

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

@eiriktsarpalis how do longs get serialized in STJ? Are they guaranteed to roundtrip?

Copy link
Contributor Author

@davidwengier davidwengier Jul 15, 2024

Choose a reason for hiding this comment

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

interesting. would like to know more about this. when are you guuys pinning solutions?

hopefully never? This is just a version of RazorPinnedSolutionInfoWrapper, which is really just Checksum, but this serializes with STJ and that uses messagepack. I could add STJ attributes to RazorPinnedSolutionInfoWrapper, but then I'd need them on Checksum too.

This type is only used to call OOP, and is then passed back to roslyn to get the real solution

Copy link
Member

Choose a reason for hiding this comment

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

@eiriktsarpalis how do longs get serialized in STJ? Are they guaranteed to roundtrip?

They serialize to JSON numbers which have arbitrary precision. They are guaranteed to roundtrip.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@davidwengier davidwengier merged commit 70a4dc0 into dotnet:main Jul 15, 2024
25 checks passed
@davidwengier davidwengier deleted the RemoteSTJServices branch July 15, 2024 23:44
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 15, 2024
davidwengier added a commit to dotnet/razor that referenced this pull request Jul 19, 2024
Requires dotnet/roslyn#74280, and won't even
compile without it.
Part of #9519

This brings signature help to Cohosting. It's a pretty simply PR, for a
pretty simple endpoint, as we just delegate, and there is no translation
of delegated info. The interesting part here is that we use
`System.Text.Json` for the remote signature help service, because it
makes more sense to take advantage of the existing Json converters for
the potential complexity of the `SignatureHelp` result type.
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 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.

4 participants