dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.04k stars 1.73k forks source link

Add Maui default global usings in SDK targets #1564

Closed mhutch closed 2 years ago

mhutch commented 3 years ago

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

Redth commented 3 years ago

@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 commented 3 years ago

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: https://github.com/xamarin/xamarin-android/issues/6075

Clancey commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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

mhutch commented 3 years ago

So using the relationships defined in https://github.com/dotnet/maui/pull/1611/commits/f82db030aaef29839ea0b43b33dd8efa29475b99 (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 commented 3 years ago

@mhutch that looks right to me.

mhutch commented 3 years ago

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: https://github.com/dotnet/sdk/issues/18825#issuecomment-877370194. Note that web does include its .Hosting namespace.

Clancey commented 3 years ago

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

mhutch commented 3 years ago

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

saint4eva commented 3 years ago

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 commented 3 years ago

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

mattleibow commented 3 years ago

I think this has recently changed a bit.

jamesmontemagno commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Was discussing my PR - > https://github.com/dotnet/maui/pull/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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

Sorry this should have been closed with https://github.com/dotnet/maui/pull/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 commented 2 years ago

@jonathanpeppers So Comet should use Xaml?

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

jonathanpeppers commented 2 years ago

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

Redth commented 2 years ago

Suggestions:

jfversluis commented 2 years ago

Is there still work to be done here?

jonathanpeppers commented 2 years ago

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!