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

EnableRequestDelegateGenerator by default for PublishTrimmed apps #48416

Closed
1 task done
eerhardt opened this issue May 24, 2023 · 5 comments · Fixed by dotnet/sdk#34022
Closed
1 task done

EnableRequestDelegateGenerator by default for PublishTrimmed apps #48416

eerhardt opened this issue May 24, 2023 · 5 comments · Fixed by dotnet/sdk#34022
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg NativeAOT

Comments

@eerhardt
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Currently ASP.NET apps have the following source generators enabled by default when PublishAot=true:

  • EnableRequestDelegateGenerator
  • EnableConfigurationBindingGenerator

These source generators are also valuable for PublishTrimmed=true apps that don't use Native AOT because they eliminate trim warnings in your app.

Describe the solution you'd like

We should enable these source generators by default when PublishTrimmed=true. Just like we do for PublishAot.

Additional context

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 24, 2023
@ghost ghost added the NativeAOT label May 24, 2023
@davidfowl
Copy link
Member

YESSSS

@mitchdenny
Copy link
Member

mitchdenny commented May 25, 2023

This is a pretty big step. We'll probably want to look at this after things like the following edge case: #47339

@davidfowl
Copy link
Member

davidfowl commented May 25, 2023

Agreed. It might be worth comparing what breaks outright today without the source generator with trimming on.

@eerhardt does ASP.NET Core do aggressive trimming by default now in .NET 8 (Not partial)? If yes, then it's worth exploring this change. If no, then maybe we shouldn't make this change in .NET 8.

@eerhardt
Copy link
Member Author

does ASP.NET Core do aggressive trimming by default now in .NET 8 (Not partial)?

Not yet. That is being decided with dotnet/sdk#30059.

There are efforts being made to push towards "full" trimming though. The current usage of partial trimming is old/legacy and really only meant for existing Mono-based apps - Blazor WASM, MAUI, etc. @agocke @DamianEdwards and I had a discussion a couple weeks ago about being consistent across defaults, docs, and UI (ex. property page in VS).

Overall, we are trying to get to an understandable and maintainable place with regards to trimming. Using partial trimming and ignoring the trim warnings is not that place.

See also dotnet/runtime#84378

@davidfowl
Copy link
Member

We should do this only if TrimMode is full.

@mkArtakMSFT mkArtakMSFT added area-runtime and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jun 1, 2023
@amcasey amcasey added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg NativeAOT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@davidfowl @mitchdenny @eerhardt @amcasey @mkArtakMSFT and others