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

Emit host outputs #8242

Merged
merged 26 commits into from
Jun 8, 2023
Merged

Emit host outputs #8242

merged 26 commits into from
Jun 8, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Feb 8, 2023

Making the source generator emit design-time C# and HTML via host outputs.

Based on

Blocked by

@jjonescz jjonescz marked this pull request as ready for review February 8, 2023 11:10
@jjonescz jjonescz requested review from a team as code owners February 8, 2023 11:10
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

One question about the NuGet.config, probably want someone with more compiler knowledge for the important bits.

NuGet.config Outdated Show resolved Hide resolved
@chsienki
Copy link
Contributor

chsienki commented Feb 8, 2023

This is an elegant approach, but it's not going to work for us unfortunately. We need to be able to produce both the design time and runtime documents for hot reload at the same time. Instead, we should fork the graph before emitting and produce both designtime for the host outputs, and runtime for the regular source.

That's obviously going to be doing more work, so lets switch it to ImplementationOnlyOutput. Then we can hook that up on the IDE side so that, effectively, the design time only runs in the IDE, the runtime only run in the command line compiler, and both run in hot reload scenarios.

Note also this isn't actually going to do anything until we enable it on the IDE side.

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Need both design time and runtime

@jjonescz jjonescz marked this pull request as draft February 9, 2023 08:47
@jjonescz jjonescz marked this pull request as ready for review February 9, 2023 10:22
NuGet.config Outdated Show resolved Hide resolved
@jjonescz jjonescz force-pushed the emit-host-outputs branch 2 times, most recently from cf0ed3f to acc14c4 Compare February 13, 2023 15:56
@jjonescz
Copy link
Member Author

jjonescz commented Feb 14, 2023

@chsienki @dotnet/razor-compiler feedback addressed, CI is green. However, we might want to first merge

@adrianwright109
Copy link
Contributor

@chsienki @dotnet/razor-compiler feedback addressed, CI is green. However, we might want to first merge

@jjonescz Perf/generator #8212 is targeting release/dev17.6 not main like this and the other associated PRs.

@jjonescz
Copy link
Member Author

@adrianwright109

Perf/generator #8212 is targeting release/dev17.6 not main like this and the other associated PRs.

True, but release/dev17.6 is periodically merged back into main, I think.

@333fred
Copy link
Member

333fred commented Mar 23, 2023

Test insertion link (internal only): https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/460368

@jjonescz
Copy link
Member Author

@chsienki for another look

@chsienki
Copy link
Contributor

New test insertion link (internal only) https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/467451

eng/Version.Details.xml Outdated Show resolved Hide resolved
@jjonescz jjonescz force-pushed the emit-host-outputs branch from 3cda258 to e3bfb62 Compare April 28, 2023 12:44
@chsienki
Copy link
Contributor

chsienki commented May 2, 2023

@jjonescz We discussed in the working group about if we want to put this work in a branch or in main behind a flag. I think this work is good for main-behind-a-flag. Can we do something similar to how we suppress the whole generator with the .razorencconfig to make this opt-in?

Basically cut out early and do no host output work unless an option in the config is present? That way it'll be off by default but we can opt-in individually for testing?

@jjonescz jjonescz force-pushed the emit-host-outputs branch from a05a140 to 6804dee Compare May 3, 2023 18:43
Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@jjonescz jjonescz added this to the 17.7 P3 milestone Jun 8, 2023
@jjonescz jjonescz merged commit a0d029c into dotnet:main Jun 8, 2023
@jjonescz jjonescz deleted the emit-host-outputs branch June 8, 2023 16:43
@chsienki
Copy link
Contributor

chsienki commented Jun 8, 2023

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants