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

Add default global usings to Microsoft.NET.Sdk.Web #32451

Closed
halter73 opened this issue May 6, 2021 · 19 comments · Fixed by dotnet/sdk#18459
Closed

Add default global usings to Microsoft.NET.Sdk.Web #32451

halter73 opened this issue May 6, 2021 · 19 comments · Fixed by dotnet/sdk#18459
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-minimal-hosting
Milestone

Comments

@halter73
Copy link
Member

halter73 commented May 6, 2021

This is one of the bullet points from #30580. A long list of mostly the same using statements at the top of every ASP.NET Core is intimidating and ultimately unhelpful noise for the most point.

We should consider a set of name spaces that will be imported by default into all cs file in net6.0 projects targeting Microsoft.NET.Sdk.Web.

Here's a strawman proposal for the possible default namespaces to use:

[Edit: updated list as of 6/23]

global using global::System;
global using global::System.Collections.Generic;
global using global::System.Linq;
global using global::System.Net.Http;
global using global::System.Net.Http.Json;
global using global::System.Threading.Tasks;
global using global::Microsoft.AspNetCore.Builder;
global using global::Microsoft.AspNetCore.Hosting;
global using global::Microsoft.AspNetCore.Http;
global using global::Microsoft.AspNetCore.Mvc;
global using global::Microsoft.AspNetCore.Routing;
global using global::Microsoft.Extensions.Configuration;
global using global::Microsoft.Extensions.DependencyInjection;
global using global::Microsoft.Extensions.Hosting;
global using global::Microsoft.Extensions.Logging; 

It's kinda crazy seeing .cs files without usings but I like it a lot.

We of course do need to be careful not to overdo this and introduce a bunch of conflicts or make intellisense too confusing. It's possible the strawman proposal is already a bit much, but I need to play with it more to get a good feeling for how convenient and/or annoying it is.

@halter73 halter73 changed the title Add default global using to Microsoft.NET.Sdk.Web for .NET 6 Add default global using to Microsoft.NET.Sdk.Web May 6, 2021
@halter73 halter73 changed the title Add default global using to Microsoft.NET.Sdk.Web Add default global usings to Microsoft.NET.Sdk.Web May 6, 2021
@benaadams
Copy link
Member

global using System.Linq; 🙄

Also global using Microsoft.EntityFrameworkCore; could that be added to subpackages in some way instead? So its included if you add a provider to your project rather than by default if you are going another way (which could even be EF6 since that now runs on .NET6.0 and I assume most of the types will conflict and lead to much confusion)

@davidfowl
Copy link
Member

Those shouldn't be in the ASP.NET one.

@davidfowl
Copy link
Member

cc @DamianEdwards

@pranavkm
Copy link
Contributor

pranavkm commented May 6, 2021

Do these play well if a file already includes these usings?

@davidfowl
Copy link
Member

Not yet (https://github.com/davidfowl/CommunityStandUpMinimalAPI/blob/386a0c1fef756e55a999429b8f8cba400ff3f4ca/Sample/Sample.csproj#L6).

I think it's interesting to discuss whether we want this in each project on on by default. Assuming we can stop the warning, I think we'd want a subset of namespaces in there by default.

@DamianEdwards
Copy link
Member

DamianEdwards commented May 6, 2021

Are we attempting to optimize for the top-level statements file or all files in the project? Today I feel the most friction is the amount of namespaces needed just to bootstrap a basic app in its Program/Startup classes and of course over time more instances of those will become merged into the top-level statements file using the minimal hosting API.

We also need to consider how this impacts existing apps that are simply used with the new SDK or just updated to target net6.0 without necessarily changing to use top-level statements or the minimal hosting API.

@halter73
Copy link
Member Author

halter73 commented May 6, 2021

I would like to optimize for all files in a common ASP.NET Core project. I don't think there's a way to limit it to certain files.

@BrennanConroy
Copy link
Member

BrennanConroy commented May 7, 2021

Triage: We should consider having an msbuild property you can put in your csproj to disable the default usings. Or opt-in.

@BrennanConroy BrennanConroy added this to the 6.0-preview5 milestone May 7, 2021
@halter73 halter73 removed this from the 6.0-preview5 milestone Jun 10, 2021
@BrennanConroy BrennanConroy added this to the 6.0-preview7 milestone Jun 14, 2021
@JunTaoLuo JunTaoLuo self-assigned this Jun 16, 2021
@JunTaoLuo
Copy link
Contributor

In the meeting we also discussed the idea of an extensibility model. There would be a mechanism in the SDK, likely a well known item group, that specifies additional global usings which will be added by the SDK to the project's obj directory. This way, additional packages, such as EF can opt into adding global usings to a project directory when the package is referenced by including a props/target file.

@JunTaoLuo
Copy link
Contributor

I've added a POC for this at dotnet/sdk#18459. To see how this looks in a Minimal API web app, checkout https://github.com/JunTaoLuo/GlobalUsings.

One of the annoying things is that the global using now conflicts with other generated cs files and create these warnings:

C:\gh\tp\GlobalUsing\App\obj\Debug\net6.0\.NETCoreApp,Version=v6.0.AssemblyAttributes.cs(2,7): warning CS0105: The using directive for 'System' appeared previously in this namespace [C:\gh\tp\GlobalUsing\App\App.csproj]
C:\gh\tp\GlobalUsing\App\obj\Debug\net6.0\App.AssemblyInfo.cs(10,7): warning CS0105: The using directive for 'System' appeared previously in this namespace [C:\gh\tp\GlobalUsing\App\App.csproj]

I'm not sure the best way to get rid of these warnings. I'm not a fan of ignoring all CS0105 warnings.

@davidfowl
Copy link
Member

It's going to be fixed by the compiler.

@JunTaoLuo
Copy link
Contributor

Also, I'm a little concerned with the serviceability of this feature. After we ship the first set of global usings, any addition or removal of using statements will be breaking right?

@davidfowl
Copy link
Member

Removal is adding isn't

@JunTaoLuo
Copy link
Contributor

Adding could introduce type conflicts no?

@davidfowl
Copy link
Member

Yes that is a good point, though there's guidance on how to avoid that generally. This could make the problem worse.

@DamianEdwards
Copy link
Member

As discussed today in sync, any changes in the future should be behind a TFM version check so that simply getting a new SDK would not introduce issues. New platform (TFM) versions for apps typically involve changes and caveats beyond the TFM change itself which is why there are migration docs for each version change and a change to default global usings would be one such change that's called out, with mitigations detailed such as disabling the feature via an MSBuild property, etc.

@davidfowl
Copy link
Member

Current list of usings:

global using global::System;
global using global::System.Collections.Generic;
global using global::System.Linq;
global using global::System.Net.Http;
global using global::System.Net.Http.Json;
global using global::System.Threading.Tasks;
global using global::Microsoft.AspNetCore.Builder;
global using global::Microsoft.AspNetCore.Hosting;
global using global::Microsoft.AspNetCore.Http;
global using global::Microsoft.AspNetCore.Mvc;
global using global::Microsoft.AspNetCore.Routing;
global using global::Microsoft.Extensions.Configuration;
global using global::Microsoft.Extensions.DependencyInjection;
global using global::Microsoft.Extensions.Hosting;
global using global::Microsoft.Extensions.Logging; 

@Pilchie
Copy link
Member

Pilchie commented Jun 24, 2021

I suggest making this set an ItemGroup in MSBuild so that people can use Remove to remove individual items if they cause type conflicts. Visual Basic has this concept (though as command line arguments, instead of in source), and they represent them as Items, with a property for a global switch.

See https://github.com/dotnet/sdk/blob/cfc3463578c94dba51115d8fc218a4a7d96281c2/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.VisualBasic.targets#L13-L39

@JunTaoLuo JunTaoLuo added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 29, 2021
@JunTaoLuo
Copy link
Contributor

For API review:

Current list of global usings:

//Sdk.Web
global using global::System;
global using global::System.Collections.Generic;
global using global::System.Linq;
global using global::System.Net.Http;
global using global::System.Net.Http.Json;
global using global::Microsoft.AspNetCore.Builder;
global using global::Microsoft.AspNetCore.Hosting;
global using global::Microsoft.AspNetCore.Http;
global using global::Microsoft.AspNetCore.Routing;
global using global::Microsoft.Extensions.Configuration;
global using global::Microsoft.Extensions.DependencyInjection;
global using global::Microsoft.Extensions.Hosting;
global using global::Microsoft.Extensions.Logging;
//Sdk.Worker
global using global::Microsoft.Extensions.Configuration;
global using global::Microsoft.Extensions.DependencyInjection;
global using global::Microsoft.Extensions.Hosting;
global using global::Microsoft.Extensions.Logging;

These usings will be generated by the .NET SDK in the obj folder by default for C# projects > net6.0. This feature can be disabled completely via EnableDefaultGlobalUsings or selectively via EnableDefaultGlobalUsings_Web/EnableDefaultGlobalUsings_Worker.

@pranavkm pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 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-minimal-hosting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants