-
Notifications
You must be signed in to change notification settings - Fork 196
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
[WIP] Get generated docs from the generator #8678
base: main
Are you sure you want to change the base?
Conversation
Emit design-time outputs from the source generator Emit host outputs from the source generator Generate both design-time and runtime outputs Switch to implementation output
Theres still a lot of 'vestigial' code in here from different approaches that I took, so I'll try and keep cleaning it up a bit as I go. I'll probably re-base and open a new PR for actual review, but feel free to leave any comments you have as you see them. |
RequestedOutput = name, | ||
}; | ||
|
||
var response = await _notifier.SendRequestAsync<HostOutputRequest, HostOutputResponse>(RazorLanguageServerCustomMessageTargets.RazorHostOutputsEndpointName, request, CancellationToken.None); |
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.
FYI, there needs to be TypeScript changes for any use of ClientNotifierService (if you expect it to work in VS Code), so either we need to do that so we don't break OmniSharp, or we have to make all of this behaviour optional and leave omnisharp using the old code. Obviously doesn't need an answer now, but hopefully one of those two things is on the todo list
namespace Microsoft.CodeAnalysis.Razor.Workspaces; | ||
|
||
[ExportWorkspaceServiceFactory(typeof(IRazorGeneratedDocumentProvider), ServiceLayer.Default)] | ||
internal class InProcRazorGeneratedDocumentProviderFactory : IWorkspaceServiceFactory |
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.
What is this fella and what does he do? It says InProc and is a roslyn service, but then is using LSP to call into Roslyn, which seems at odds with that. Can't it just pull generated files out of the Project?
UPDATE: Wait, maybe this isn't using LSP. Is RazorHostOutputHandler
a Roslyn thing that we're accessing over IVT?
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, exactly that. There's just a static method that we can call via IVT that pulls the docs out of the generator workspace. I'll separate out the types to make it clearer.
{ | ||
public override DirectiveTokenDescriptor? ReadJson(JsonReader reader, Type objectType, DirectiveTokenDescriptor? existingValue, bool hasExistingValue, JsonSerializer serializer) | ||
{ | ||
var obj = JObject.Load(reader); |
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.
I know this is a prototype, but these serializers definitely need to be looked at for performance before putting them in the product. I'm currently rewriting the existing JSON.NET converters in the tooling layer because they do horrible inefficient things like using JObject
within a converter.
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, right now the serialization is not great. My plan was to get it to point where we can profile it, then start improving, although it sounds like you're already re-writing some of them anyway.
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.
Yeah, it'd be good to get us to a place where the serialization logic can just go into Shared.Utilities. TagHelperDescriptor serialization currently represents a large amount of memory allocation in Razor scenarios. So, there's already a fair amount of analysis that's been done around what's good/bad with JSON.NET. #8675 rewrites the existing JSON.NET converters in the tooling layer and avoids changing behavior, since we still have to read the project.razor.json files that are already in the wild. Once this goes in, I'll introduce versioning into serialization so that we can use a much tighter format.
See #8113 for some of the details.
private readonly int rewritePhaseIndex = -1; | ||
|
||
public SourceGeneratorProjectEngine(DefaultRazorProjectEngine projectEngine) | ||
: base(projectEngine.Configuration, projectEngine.Engine, projectEngine.FileSystem, projectEngine.ProjectFeatures) |
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.
Tabs vs. Spaces?
@@ -218,6 +218,7 @@ private static void WriteAllowedChildTags(JsonWriter writer, AllowedChildTagDesc | |||
writer.WriteEndObject(); | |||
} | |||
|
|||
// TODO: looks like we have bound attributes not part of tag helpers. We need to extract this to be an actual type converter and call into it here instead. | |||
private static void WriteBoundAttribute(JsonWriter writer, BoundAttributeDescriptor boundAttribute, JsonSerializer serializer) |
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.
This is all being rewritten. 😄
Heres a (very) WIP PR for the work I've been doing to get the docs from the generator. It builds on top of a couple of other PRs, but should give you a feel for the basic shape.
Conceptually it's not too complicated, there are two new services, one InProc, one LSP that can talk to the matching Roslyn endpoint and pull out the docs.