-
Notifications
You must be signed in to change notification settings - Fork 198
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
Create a consistent and reliable API between Razor IDE and compiler #11182
Comments
Didn't we want to use the razor source generator as the source of all compiler truth, and tooling just reading its outputs (#6919)? Then those outputs would be the only API between tooling and compiler, I presume. |
Yes, thats what cohosting will deliver, and will make this issue moot, but thats also not a short term thing. |
@davidwengier out of interest when is co-hosting expected to be delivered? Secondly what benefit(s) will us Visual Studio and/or VS Code users see in our day to day development loop using it? |
When is a very tough question to answer. We're about 90% of the way through porting each individual Razor feature over (#10364) but there is one particularly big rock that needs to move (turning on the source generator in the IDE) and that will probably uncover a lot of issues in Roslyn (eg dotnet/roslyn#72136, #10856, #10864, #10865 and a lot of as-yet-discovered issues I'm sure). As for benefits, the main things to start with are performance and reliability. Performance because right now Razor runs its own project system, and has hooks into the .NET project system, and Roslyn, that get data and respond to events, and send it into Razor so it can be absorbed into it's project system etc. With cohosting Razor will essentially exist inside Roslyn, so all of that work will have already been done and we're just re-using the existing Roslyn project system. Reliability, and some performance, comes from Razor not having to compile the generated C# files and keep them synchronized with Roslyn, and make sure when we ask Roslyn about them we've send the right version across etc. because, again, everything will be in the same world. In future we can then build on it to get more features available in Razor too. eg, because our current communication method with Roslyn is LSP, we essentially have a string based API which limits what we can do. When we have cohosting we can upgrade some of those APIs to talk in terms of Roslyn data structures, like |
@davidwengier thanks for the detailed explanation. |
Preparation for ongoing work to hook up the Roslyn tokenizer and #11182 I suppose. There were three places that `UpdateProjectWorkspaceState` was called: 1. In `RazorProjectService`, just before calling `UpdateProjectConfiguration` 2. In `ProjectWorkspaceStateGenerator`, where we will need to add a call to `UpdateProjectConfiguration` in future, to wire up the tokenizer 3. In our LiveShare bits, in response to events from the above. Previous attempts to plumb through more things for `RazorConfiguration` resulted in RPS failures, that appeared to be simply more compilations of closed files. This makes sense because we were adding another update, which would have triggered another set of `ProjectChanged` events. I thought it would make more sense to combine these two updates together, so no matter which part of the project was being updated, there could be a single `ProjectChanged` notification. This is that.
Right now the IDE configures the compiler by:
RazorCongfiguration
which has various propertiesThis has all generally been a bug farm in the past, makes it easy for things to be missed, and means there is no good way for someone working in the compiler to know if they will need IDE support for their work, nor someone working in the IDE know what they need to provide the compiler in order to get a coherent experience.
If we could centralise on a single strongly typed API that contained everything the Razor compiler needs from the IDE, it should make these issues go away. The simplest idea, from an IDE perspective, would be to add more parameters to the
RazorConfiguration
constructor, and get rid of theconfigure
lambda entirely, and have that be the contract. Open to all thoughts./cc @DustinCampbell @333fred @chsienki
The text was updated successfully, but these errors were encountered: