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

Set InvariantGlobalization in api template #47066

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Mar 7, 2023

API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix #47029

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 7, 2023
eerhardt added a commit to eerhardt/Benchmarks that referenced this pull request Mar 7, 2023
The api template is updating, see dotnet/aspnetcore#47066. This syncs that change here.
@mitchdenny
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

@captainsafia
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

I went ahead and took care of this in dotnet/msbuild#8543 since it was fairly straightforward to do.

@DamianEdwards
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

I went ahead and took care of this in dotnet/msbuild#8543 since it was fairly straightforward to do.

Right, we just need to follow up on VS property page support.

JamesNK
JamesNK previously requested changes Mar 8, 2023
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

Update other templates? webapi, grpc, worker?

Figure out how to test that culture is disabled. I'm not sure how to easily do this. The default template endpoints are returning invariant data already.

API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix dotnet#47029
@eerhardt eerhardt requested a review from JamesNK March 9, 2023 23:38
@eerhardt eerhardt dismissed JamesNK’s stale review March 10, 2023 15:03

Responded to feedback

@eerhardt eerhardt merged commit 53e77ee into dotnet:main Mar 10, 2023
@eerhardt eerhardt deleted the UseInvariantGlobalization branch March 10, 2023 21:25
@ghost ghost added this to the 8.0-preview3 milestone Mar 10, 2023
eerhardt added a commit to aspnet/Benchmarks that referenced this pull request Mar 10, 2023
* Set InvariantGlobalization in BasicMinimalApi and BasicGrpc apps

The api template is updating, see dotnet/aspnetcore#47066. This syncs that change here.
@Varorbc
Copy link
Contributor

Varorbc commented May 11, 2023

@eerhardt If enabled by default,SqlClient will throw an exception

@ghost
Copy link

ghost commented May 11, 2023

Hi @Varorbc. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@eerhardt
Copy link
Member Author

@eerhardt If enabled by default,SqlClient will throw an exception

The simple workaround in that case is to just remove this line from the .csproj then.

@ghost
Copy link

ghost commented May 11, 2023

Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@AlmightyLks
Copy link

AlmightyLks commented Nov 25, 2023

FYI:
Breaks usage with EFC (Or SqlClient?) and thus makes ASP.NET Core 8 incompatible with EFC (/ SqlClient) by default

@ghost
Copy link

ghost commented Nov 25, 2023

Hi @AlmightyLks. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@roji
Copy link
Member

roji commented Nov 27, 2023

FYI this is purely a SqlClient-related issue that has nothing to do with EF per se.

@ghost
Copy link

ghost commented Nov 27, 2023

Hi @roji. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider setting InvariantGlobalization in api template
8 participants