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 Maui default global usings in SDK targets #1564

Closed
mhutch opened this issue Jul 6, 2021 · 31 comments
Closed

Add Maui default global usings in SDK targets #1564

mhutch opened this issue Jul 6, 2021 · 31 comments
Assignees
Labels
area-templates Project templates, Item Templates for Blazor and MAUI
Milestone

Comments

@mhutch
Copy link
Member

mhutch commented Jul 6, 2021

Following the pattern in dotnet/sdk#18459, we should add default global usings for Maui in the SDK targets and the corresponding MSBuild property to disable them.

@Redth
Copy link
Member

Redth commented Jul 7, 2021

@jonathanpeppers are you able to assist on this?

For MAUI I think we need:

using Microsoft.Maui;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Controls;

The last one (Controls) maybe is worth debating...

@jonathanpeppers
Copy link
Member

I don't think the changes in dotnet/sdk have landed yet, but we can try it soon, I think?

You will be able to put this in a .targets file:

<ItemGroup>
  <Import Include="Microsoft.Maui" />
  <Import Include="Microsoft.Maui.Controls" />
</ItemGroup>

Let me do the Android side first, and I'll know how it works: dotnet/android#6075

@Clancey
Copy link
Contributor

Clancey commented Jul 7, 2021

Right now, if you are trying to use something like Comet, and you have using Microsoft.Maui.Controls as well, it will cause a lot of confusion. Since using Microsoft.Maui.Controls.Button and Comet.Button

@Redth
Copy link
Member

Redth commented Jul 8, 2021

@Clancey yeah that's what I was thinking too. Perhaps we should leave controls out then, or better yet we can conditionally put that one in the item group if the controls stuff is referenced?

@jonathanpeppers did we ever make a property to exclude Maui controls references explicitly? Do we need one and put this behind it as well?

@jonathanpeppers
Copy link
Member

Yes, each global using could be split up across parts of Maui where it makes sense. I don’t think you have an explicit thing to exclude Controls, but UseMauiCore, UseMauiAssets, and UseMauiEssentials (and not UseMaui) would exclude Controls.

@saint4eva
Copy link

@jonathanpeppers are you able to assist on this?

For MAUI I think we need:

using Microsoft.Maui;
using Microsoft.Maui.Graphics;
using Microsoft.Maui.Controls;

The last one (Controls) maybe is worth debating...

The last one (Controls) should not be included by default since there are other app models such as Comet, Fabolous and Reactive UI

@dotnet dotnet deleted a comment Jul 8, 2021
@mhutch
Copy link
Member Author

mhutch commented Jul 9, 2021

So using the relationships defined in f82db03 (UseMaui sets UseMauiCore and _UseMauiControls) it would look something like this?

<PropertyGroup Condition="'$(DisableImplicitNamespaceImports_Maui)' != 'true'">
  <Import Include="Microsoft.Maui" Condition="'$(UseMauiCore)'=='true'" />
  <Import Include="Microsoft.Maui.Graphics" Condition="'$(UseMauiCore)'=='true'" />
  <Import Include="Microsoft.Maui.Controls" Condition="'$(_UseMauiControls)'=='true'" />
</PropertyGroup>

@jonathanpeppers
Copy link
Member

@mhutch that looks right to me.

@mhutch
Copy link
Member Author

mhutch commented Jul 9, 2021

The Maui template's Startup.cs has the following usings:

using Microsoft.Maui;
using Microsoft.Maui.Hosting;
using Microsoft.Maui.Controls.Compatibility;
using Microsoft.Maui.Controls.Hosting;
using Microsoft.Maui.Controls.Xaml;

And default usings will only get rid of one of these. Should we be adding more of them?

For reference, here's what the base and web SDKs add: dotnet/sdk#18825 (comment). Note that web does include its .Hosting namespace.

@Clancey
Copy link
Contributor

Clancey commented Jul 9, 2021

using Microsoft.Maui.Hosting; is only used in Startup. The rest are for Controls only. They would cause issues if in Comet.

@mhutch
Copy link
Member Author

mhutch commented Jul 10, 2021

Sure, but the hosting one would be consistent with web, and the rest would be conditioned on $(_UseMauiControls)

@saint4eva
Copy link

The Maui template's Startup.cs has the following usings:

using Microsoft.Maui;
using Microsoft.Maui.Hosting;
using Microsoft.Maui.Controls.Compatibility;
using Microsoft.Maui.Controls.Hosting;
using Microsoft.Maui.Controls.Xaml;

And default usings will only get rid of one of these. Should we be adding more of them?

For reference, here's what the base and web SDKs add: dotnet/sdk#18825 (comment). Note that web does include its .Hosting namespace.

Hmmm Xaml? Remember other app mdels such as Comet and Fabulous. Thank you.

@mhutch
Copy link
Member Author

mhutch commented Jul 12, 2021

@saint4eva that's why I suggested conditioning the last 3 on $(_UseMauiControls), which basically means xaml

@mattleibow mattleibow added this to the 6.0.100-rc.1 milestone Aug 15, 2021
@mattleibow
Copy link
Member

I think this has recently changed a bit.

@Eilon Eilon added the area-templates Project templates, Item Templates for Blazor and MAUI label Sep 20, 2021
@jamesmontemagno
Copy link
Member

Here are my recommendations:

global using Microsoft.Extensions.DependencyInjection;
global using Microsoft.Maui;
global using Microsoft.Maui.Controls;
global using Microsoft.Maui.Controls.Hosting;
global using Microsoft.Maui.Controls.Xaml;
global using Microsoft.Maui.Essentials;
global using Microsoft.Maui.Hosting;

@dotMorten
Copy link
Contributor

What if the underlying SDKs that Maui wraps also does this (which is likely with the trend this is setting)? There seem to be a huge potential for class name collisions then. For instance if WinUI does the same thing, suddenly we can't resolve Grid when targeting Windows, but would work when compiling for iOS and Android (and similar issues could happen on those platforms).

IMHO makes more sense to just do this at the template level, and leave the global default usings to the very basic System classes.

@jamesmontemagno
Copy link
Member

Gotcha, yeah that is a good point about controls themselves that have overlap with native platforms under the hood. So we should think about which would not cause issues and which make sense to bring in. Although ImplicitUsings are opt-in so developers could turn them off at any time too if they don't want them.

I would need to do some more research here, but our hope is that you wouldn't be writing much if any native code. For library creators you would turn it off if you were making custom controls.

🤔🤔🤔

@dotMorten
Copy link
Contributor

@jamesmontemagno I'm guessing if using the <Imports /> approach, it is possible to also remove some. So the Maui SDK could explicitly remove WinUI imports to avoid this, but it should probably require some coordination with various SDKs to ensure they don't step on each-other's feet. I am however worried this gets unwieldy quick if every single SDK and NuGet Package out there starts littering global usings. You could literally break your build just by adding a NuGet package.

ImplicitUsings are opt-in

Sure, but I might want to use it, but just the limited normal .NET System.* set, rather than an all-or-nothing thing. When all the different SDKs auto-imports these, it is hard to control and remove, rather than the template explicitly sets some I can easily delete if I don't want them.

@jonathanpeppers
Copy link
Member

Sure, but I might want to use it, but just the limited normal .NET System.* set, rather than an all-or-nothing thing. When all the different SDKs auto-imports these, it is hard to control and remove, rather than the template explicitly sets some I can easily delete if I don't want them.

I think you already have the control you're asking for here.

In the case of Android:

https://github.com/xamarin/xamarin-android/blob/afae7be876c1268725d4ee418d50771006e65848/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/Sdk/AutoImport.props#L23-L27

.NET MAUI's MSBuild targets can do:

<Using Remove="Android.App" />
<Using Remove="Android.Widget" />
<Using Remove="Android.OS.Bundle" />
<Using Include="Microsoft.Maui" />
<!-- etc. -->

You can put this in your .csproj file as well if you like.

@dotMorten
Copy link
Contributor

@jonathanpeppers sure but it is not at all very discoverable and the sort of errors you'll be getting won't be that helpful to lead you there.

@jamesmontemagno
Copy link
Member

Was discussing my PR - > #3018

One of the issues we do run into if we enable ImplicitUsings in a .NET MAUI application they will trickle down to the native iOS/Android/Windows platform. Since Android brings in Android.Widget then we will get a compile error if we do:

var button = new Button();

because it is in both namespaces. So our resolve here is to disable ImplictUsing for .NET MAUI and then bring in a larger GlobalUsing.cs in the templates.

@dotMorten
Copy link
Contributor

@jamesmontemagno Probably a good choice, for now at least. I do wonder though if it makes sense that Maui would "remove" underlying namespaces, since you likely don't want to implicitly use namespaces from the native platforms. It would however require coordination between all dependent SDKs. For instance I doubt WinUI is adding this in 1.0GA, but if they do in 1.1, MAUI would quickly have to go and update to remove them.

@jamesmontemagno
Copy link
Member

jamesmontemagno commented Oct 26, 2021

@dotMorten Yeah, I think there is a bigger discussion to have around SDK stuff as well. We were talking today in our team meeting about maybe we should have a
ImplicitAndroidUsings, ImplicitiOSUsings, ImplicitMauiUsings.

Thoughts on that? Where ImplicitUsings is just the core .NET ones and SDK ones are separate.

We also thought about removing them, but like you said that is a rabbit hole.

@hartez
Copy link
Contributor

hartez commented Jan 4, 2022

@jamesmontemagno @mhutch @jonathanpeppers The discussion on this seems to have fizzled out - is this still an issue we need to work out, or can we close it?

@Clancey
Copy link
Contributor

Clancey commented Jan 4, 2022

Honestly no UI Framework should auto add their controls via Implicit Usings. They should be done via a GlobalUsings.cs in the template. The reason being is name collisions. Pretty much every UI Framework has a Button. When you are doing cross platform in Maui, it all falls down. Then add Comet on top of Maui and implicit using is required to be disabled. Horrible/inconstant behavior.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jan 5, 2022

Sorry this should have been closed with #3267, I didn't know (forgot?!) there was an issue for it.

@Clancey Comet has the flexibility to do what it needs from MSBuild targets, such as what MAUI had to do to clear the platform-specific implicit usings from the iOS, Android, etc. workloads:

<ItemGroup Condition=" '$(UseMaui)' == 'true' and '$(MauiEnablePlatformUsings)' != 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
  <Using Remove="@(Using->HasMetadata('Platform'))" />
</ItemGroup>

When I added the MAUI usings in #3267, I added Sdk="Maui" metadata, so you could clear them in a similar fashion:

<ItemGroup Condition=" '$(UseMaui)' == 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
  <!-- %(Sdk) is only here if something later needs to identify these -->
  <Using Include="Microsoft.Extensions.DependencyInjection" Sdk="Maui" />
  <Using Include="Microsoft.Maui" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Controls" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Controls.Hosting" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Controls.Xaml" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Graphics" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Essentials" Sdk="Maui" />
  <Using Include="Microsoft.Maui.Hosting" Sdk="Maui" />
</ItemGroup>

Can be cleared with:

<ItemGroup Condition=" '$(CometEnableMauiUsings)' != 'true' and ('$(ImplicitUsings)' == 'true' or '$(ImplicitUsings)' == 'enable') ">
  <Using Remove="@(Using->WithMetadataValue('Sdk', 'Maui'))" />
</ItemGroup>

$(CometEnableMauiUsings) is a way for someone to turn off the removal, which maybe you don't need.

@mhutch can correct me if I'm wrong, but I think the guidance is that NuGet packages should not add implicit usings (only "in the box" .NET workloads should).

So if that is the case, Comet could either:

  1. Evaluate whether Comet projects should set UseMaui=true. Maybe they don't and so you have no problem?
  2. Clear out any implicit MAUI usings and just use the System ones from the .NET SDK.
  3. Don't put ImplicitUsings=enable in any Comet templates? Would be a strong suggestion not to use them.

I think you have the flexibility to do whatever you like here. There might be some future design for implicit global usings, but I don't know when that will be coming.

Feel free if someone has better MSBuild ideas here -- what we have here is what we could come up with in .NET 6, and there is probably a better general solution here.

@saint4eva
Copy link

@jonathanpeppers So Comet should use Xaml?

<Using Include="Microsoft.Maui.Controls.Xaml" Sdk="Maui" />

@jonathanpeppers
Copy link
Member

I think Comet should remove any implicit usings it doesn't need inside Comet's MSBuild targets.

@Redth
Copy link
Member

Redth commented Jan 20, 2022

Suggestions:

  • For iOS/Android usings, make metadata a bit more specific: <Using Include="" TargetPlatformIdentifier="iOS" Sdk="Microsoft.macOS.Sdk" />
  • MAUI Templates will be changed to have a GlobalUsings.cs:
<Using Include="Microsoft.Maui" Sdk="Maui" />
<Using Include="Microsoft.Maui.Controls" Sdk="Maui" />
<Using Include="Microsoft.Maui.Graphics" Sdk="Maui" />
<Using Include="Microsoft.Maui.Essentials" Sdk="Maui" />

@Redth Redth modified the milestones: 6.0.300-rc.2, 6.0.300-rc.3 Apr 20, 2022
@Redth Redth modified the milestones: 6.0.300-rc.3, 6.0.300 Apr 27, 2022
@jfversluis
Copy link
Member

Is there still work to be done here?

@jonathanpeppers
Copy link
Member

There is some other conversation here, but I think the original issue @mhutch posted is complete.

I think we can close, and if there is still an issue somewhere on here file a new one, thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-templates Project templates, Item Templates for Blazor and MAUI
Projects
No open projects
Status: Done
Development

No branches or pull requests