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

Review of the "Essentials" interface API #5407

Merged
merged 62 commits into from
Mar 26, 2022
Merged

Review of the "Essentials" interface API #5407

merged 62 commits into from
Mar 26, 2022

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Mar 18, 2022

Description of Change

As I was working through #5562, I noticed that as a result of the first pass of the Essentials API from statics to interfaces, we did not correctly updated the initial changes.

There are several cases where the API of the interface was geared towards swapping out the implementation of the static API:

interface IClipboard {
    Task<string> GetTextAsync();
    void StartListeners();
    void StopListeners();
}

However, this is incorrect and unhelpful in the new world where the interface is supposed to be self contained an mockable:

interface IClipboard {
    Task<string> GetTextAsync();
    event EventHandler<EventArgs> ClipboardContentChanged;
}

API Changes

Originally we were going to make the new API be instance based and remove (then we changed to just obsolete) the static methods, however, this is a massive change and no benefit. Sure, it is a bit weird to have a Accelerometer.Start() AND Accelerometer.Default.Start(), however, as can be seen from the diff - this means no code changes for anyone that was using essentials and then Maui.

However, with the work we have been doing with these APIs over the month, we now have a useful as well as "opt-in" way of using essentials.

Previously there was no way to mock/test using essentials, and thus you either had to roll your own abstraction or get someone else to do it.

Now, we have all that work in the box, thanks to several PRs and adjustments. And no code changes for existing usage.

Static APIs

If you are writing a simple app and don't believe in testing, you can write code like this using the static APIs Geolocation.GetLocationAsync():

class MyViewModel
{
    public async Task UploadLocationAsync()
    {
        var location = await Geolocation.GetLocationAsync();
        var uploader = MyUploader();
        await uploader.UploadLocationAsync(location);
    }
}

Instance / DI-Friendly APIs

However, if you want to create a testable system or use DI, you now can using the instance APIs Geolocation.Default.GetLocationAsync():

class MyViewModel
{
    public MyViewModel(IGeolocation geolocation, IUploader uploader)
    {
        // TODO: set fields
    }

    public async Task UploadLocationAsync()
    {
        var location = await deolocation.GetLocationAsync();
        await uploader.UploadLocationAsync(location);
    }
}

Now you an also use DI or write tests:

async Task TheBigTest()
{
    var vm = new MyViewModel(new StubGeolocation(), new StubUploader());
    await UploadLocationAsync();

    // TODO: assert location was retrieved and then uploaded
}

@mattleibow mattleibow marked this pull request as draft March 18, 2022 03:39
@mattleibow mattleibow requested review from hartez, Redth, jsuarezruiz, rmarinho and PureWeen and removed request for hartez March 18, 2022 03:39
@jsuarezruiz jsuarezruiz added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Mar 18, 2022
@mattleibow

This comment was marked as outdated.

@Redth Redth added this to the 6.0.300-rc.1 milestone Mar 21, 2022
@Redth
Copy link
Member

Redth commented Mar 21, 2022

@mattleibow I wonder if we still want all the Implementation classes to live in the .Implementation namespace?

@mattleibow
Copy link
Member Author

Right now I am moving them internal and back into the nice namespace.

In net7 we can figure out a nice way for users to set this.

The plan now is to make Accelerometer.Default return the OS accelerometer, but via an interface. If you want code to be testable, do t use the static in vms, use the interface and DI using .Default.

Think of the static as a way to get the OS instance, but in reality you need to make your apis testable, not our initial idea to make the core OS testable.

See for example things starting with A to F. Done that half first and will continue tomorrow.

@Redth
Copy link
Member

Redth commented Mar 21, 2022

I missed that you made them internal, that works for me for net6 timeframe

@Redth Redth marked this pull request as ready for review March 22, 2022 19:04
@mattleibow mattleibow changed the base branch from main to dev/essentials-ns March 25, 2022 05:11
@mattleibow mattleibow requested a review from Redth March 25, 2022 15:37
Base automatically changed from dev/essentials-ns to main March 25, 2022 17:00
@mattleibow mattleibow marked this pull request as ready for review March 25, 2022 17:17
@mattleibow mattleibow merged commit f8936a5 into main Mar 26, 2022
@mattleibow mattleibow deleted the dev/essentials-api branch March 26, 2022 00:43
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
@samhouts samhouts added the fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info fixed-in-6.0.300-rc.1 Look for this fix in 6.0.300-rc.1!
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants