Add Essentials dependency

[jfversluis]

Description of Change

Adding the dependency on Essentials, adding the first call to the RequestedTheme API and a gallery page for that.

We have decided to take on the dependency on Essentials. We are seeing more and more areas where we would have potential overlap. In order not to duplicate functionality, we simply decided to add the dependency to Essentials and make use of their awesome stuff.

Issues Resolved

• related to [Spec] OS Themes (Dark Mode support) #9804

API Changes

None

Platforms Affected

Not directly affected, but the dependency is added for these platforms to begin with:

• Core/XAML (all platforms)
• iOS
• Android
• UWP
• MacOS
• Tizen

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Take the NuGets resulting for this build, add them to a File > New project and/or existing project and see if everything proceeds to work as it did before.

PR Checklist

• Targets the correct branch
• Tests are passing (or failures are unrelated)

[dansiegel]

@jfversluis can you please elaborate as to why this is being added to the Core... that is a very dangerous call that threatens everyone who actually tests their code.

[davidortinau]

@dansiegel can you elaborate on how taking a dep on Essentials harms your ability to test your app code?

[dansiegel]

@davidortinau sure let's look at the modification to the Application class..

public class Application
{
    public AppTheme RequestedTheme => AppInfo.RequestedTheme;
}

Now let's look at the implementation from Xamarin.Essentials that will get used when we want to unit test our apps...

static AppTheme PlatformRequestedTheme() => throw ExceptionUtils.NotSupportedOrImplementedException;

https://github.com/xamarin/Essentials/blob/develop/Xamarin.Essentials/AppInfo/AppInfo.netstandard.cs#L15

Simply put there is no way to Mock this, or test any code related to it.

This is exactly why we've been saying for a long time that Xamarin.Essentials was never architected to be directly used by anyone who wants to test their code. This might be a very different conversation if the team had chosen to invest any time, effort or energy in making device runners a first class option. However since Unit Tests need to be done using either full framework or netcore either of which will ultimately use the netstandard targets of Xamarin.Essentials.

I believe a much better alternative would be to simply have a property using the AppTheme enum from Essentials but making the Xamarin.Forms platform code responsible for setting that AppTheme property in the Application class. Anything that is used in the Xamarin.Forms Core is very risky when it comes from Xamarin.Essentials.

[PureWeen]

This is exactly why we've been saying for a long time that Xamarin.Essentials was never architected to be directly used by anyone who wants to test their code.

Why would you call Essentials directly from xplat code that you are wanting to test instead of putting those calls behind an interface that you mock/control? Why do you need that abstraction to be built into Essentials for you?

If you are writing a UWP application that uses the GeoLocator class you don't need the GeoLocator class to be mockable. You'd just create a IGeoLocationService and then mock that in your xplat test. Then on the platform it would swap in a concrete implementation. Xamarin.Essentials represents concrete platform implementations why does it need to expose testable hooks for you?

I don't really follow how this has anything to do with Essentials directly. If I have a ViewModel that makes a WebRequest and I want to test that ViewModel then I will have a service that I can mock for the test. I don't need to mock HttpClient.

I do think inside Forms we are going to need to create those seams ourselves when interacting with Essentials. That was going to become apparent pretty quickly once our xplat unit tests start failing with not implemented exceptions.

We have lots of unit tests that are all NS and don't run on a specific platform. Anything that's not testable or becomes untestable because of this PR will show up with those tests.

[PureWeen]

Basically what the wiki says

https://github.com/xamarin/Essentials/wiki/FAQ-%7C-Essentials#where-are-the-interfaces

[jfversluis]

Adding DNM for now while we figure out VS2017 support

[dansiegel]

@PureWeen It's easy to leave comments on Twitter like:

Forms Core runs ~5k unit tests without any platform dependencies involved. None of those are going away.

How many of those ~5k unit tests were added around adding a property to the Application class that DIRECTLY calls the Xamarin.Essentials API? Oh yeah, that's right 0... there are ZERO Unit Tests that cover this concern.

If you are writing a UWP application that uses the GeoLocator class you don't need the GeoLocator class to be mockable. You'd just create a IGeoLocationService and then mock that in your xplat test.

This is exactly my point on what Xamarin.Forms should be doing. Even the docs are quick to point out that Essentials is architected for the consumer to abstract. In this case the consumer is Xamarin.Forms not the end app developer. It's actually quite a common and very valid issue where a developer may write code that needs to reference an actual Application class not some IWhatever. As people start to write code around AppTheme they're going to quickly discover nothing works in tests though because of how this was implemented.

We could and should also look at what platforms does Xamarin.Essentials support?

You might ask does this line up with the platforms that Xamarin.Forms supports? The answer of course is no. In other words my initial concerns remain extremely valid here due to the implementation.

[PureWeen]

Oh yeah, that's right 0... there are ZERO Unit Tests that cover this concern.

Yea @jfversluis created the PR yesterday and it's not merged and it's being actively worked on.

This is exactly my point on what Xamarin.Forms should be doing.

It will. See previous comment. If something is missed making your code untestable create an issue and we'll abstract and fix

Anything that currently exists that gets testability broken should break our xplat tests. If it doesn't then our xplat tests are lacking and we'll add more.

This is exactly why we've been saying for a long time that Xamarin.Essentials was never architected to be directly used by anyone who wants to test their code.

My comments were specifically targeting this statement about how X.E was architected to make testing impossible which is a statement I don't understand. I'm first just establishing that Essentials is fine and doesn't need interfaces and then we build out from there. If there's something fundamentally wrong with Essentials that will make it impossible for us then let's fix that.

But from your most recent comments it sounds like you're totally fine with us depending on essentials and that essentials itself is fine but that we should just be careful to not break forms xplat testability. Which we will be.

[knocte]

You might ask does this line up with the platforms that Xamarin.Forms supports? The answer of course is no.

I agree with the above sentiment ^ I myself depend on the macOS/Linux(GTK) bits of Xamarin.Forms, and given that Xamarin.Essentials doesn't support these 2 platforms, I'm using a fork. Making XF depend on Xamarin.Essentials would prevent me to upgrade to new versions of XF because the XE fork I'm using (to support macOS and Linux) would conflict with it.

[jfversluis]

You might ask does this line up with the platforms that Xamarin.Forms supports? The answer of course is no.

I agree with the above sentiment ^ I myself depend on the macOS/Linux(GTK) bits of Xamarin.Forms, and given that Xamarin.Essentials doesn't support these 2 platforms, I'm using a fork. Making XF depend on Xamarin.Essentials would prevent me to upgrade to new versions of XF because the XE fork I'm using (to support macOS and Linux) would conflict with it.

Would it help if we added support for the missing platforms? Also, since you have the fork in your apps for a while, we can probably learn a thing or two from you. So it would be awesome if you could contribute some of that so were sure that you are not left out.

[knocte]

Would it help if we added support for the missing platforms?

Yes, indeed. In fact, I'm subscribed to the macOS issue and I even provided a way to fix the build with GithubActionsCI, if you see my comment there. My fork just contains the macOS work in that PR so far, and some Linux fixes that I was going to submit as a PR as soon as the macOS PR is merged.

[dansiegel]

@jfversluis remember there is going to be one ongoing problem. Xamarin.Forms supports both WPF and GTK. To the best of my knowledge this isn't something that Xamarin.Essentials would be able to do as a library that is cross compiled with a static API rather than being based on interfaces. The problem there is that both GTK and WPF rely on the same NuGet TFM. To solve that problem would require working with some other teams across Microsoft and take time to release a solution.

Also keep in mind that you have targets like Web Assembly or Blazor which it makes no sense for Xamarin.Essentials to target. Over time we may see other targets come onto the scene like Tizen where there would be a desire support the platform with Xamarin.Forms but it may not make initial sense to support with Xamarin.Essentials.

[PureWeen]

@knocte

I agree with the above sentiment ^ I myself depend on the macOS/Linux(GTK) bits of Xamarin.Forms, and given that Xamarin.Essentials doesn't support these 2 platforms, I'm using a fork. Making XF depend on Xamarin.Essentials would prevent me to upgrade to new versions of XF because the XE fork I'm using (to support macOS and Linux) would conflict with it.

If Xamarin.Forms.Core itself never calls directly into an API that has a platform specific implementation that's missing on other platforms then would you still be broken?

If we have unit tests for all the xplat platforms apis that prove they still run if all you have is the netstandard essentials then you'll be fine right?

[knocte]

If Xamarin.Forms.Core itself never calls directly into an API that has a platform specific implementation that's missing on other platforms then would you still be broken?

I think I'd be still be broken because my fork has the same assembly name and namespace than XamarinEssentials, so if Xamarin.Forms pulls a dependency that overwrites mine, then the macOS implementation disappears, I guess (unless I figure out how to make my dependency prevail over Xamarin.Forms' dependency?).

If Xamarin.Forms.Core itself never calls directly into an API that has a platform specific implementation that's missing on other platforms then would you still be broken?

Mmm not sure. I think my point above still stands. If you add Xamarin.Essentials to XF in v.next, I think there's only 2 ways for me to be able to adopt this new version:
a) Make my fork have a different assembly name and namespace.
or:
b) Have Xamarin.Essentials maintainers merge the "unfinished" macOS (and other platforms) PRs, and throw NSE at runtime for the unfinished bits (and obviously don't announce this in the release notes, so that people use it at their own risk); then XF adopts this new XE version.

[PureWeen]

@knocte I think there are ways around that

I'm pretty sure if you added the essentials nuget to the platform head and told it to exclude all the assets and then referenced your dll that would work

I do a similar thing here where I want to reference the Xamarin Source

https://github.com/PureWeen/Xamarin.Forms.Sandbox/blob/master/Sandbox/Xamarin.Forms.Sandbox.Android/Source.targets#L3

I add the Xamarin.Forms nugets directly and set the ExcludeAssets="All" which let's me add other nuget dependencies to my head projects that transitively depend on Forms.

This way it's still using my compiled dll instead of the nuget one on the platform heads. So if you add essentials, exclude all the assets, and then add your dll then so long as the API on your Essentials package isn't different you'll be good to go.

Also, with the way we are looking at doing this I could see a scenario where you are instead swapping in your implementations via an interface provided from forms.

For example, we can't call an API directly in Forms.Core that's not implemented on GTK because than GTK will throw an exception. That means the call into essentials has to be abstracted out to an interface which could be used as the interaction point over essentials.

Like on forms we'll have

Forms.GetAppTheme()

That will go to some interface IAppThemeGetter

On platforms that essentials can take care of this we use essentials on those that don't we either roll our own or just return a default. If someone on GTK wants to provide a custom implementation for IAppThemeGetter they can.

I know that second part doesn't completely jive with what you've setup as I suspect you might be using essentials on non forms projects :-) but it'll be a way you can swap things in if needed.

[MagicAndre1981]

why are you not updating to 6.2.10? Any reason?

[jfversluis]

Is there a specific reason we should use that version? ;) I just matched the Essentials version for now.

[jfversluis]

@knocte @dansiegel with the AppTheme stuff merged it was time to give this an update.

This would more be like the final form, at least for this part.

Build is failing atm because of some other things, but you can have a glance at the code.

[jfversluis]

Change of plans, will revisit later

[MagicAndre1981]

Change of plans, will revisit later

so, XF4.7 will NOT have dependency to Essentials?

[jfversluis]

That is correct