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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

IUserDialogs is now registered in IServiceCollection automatically #2

Merged

Conversation

Axemasta
Copy link
Contributor

@Axemasta Axemasta commented Sep 4, 2023

Since the app is using a MauiAppBuilder a quality of life addition would be to automatically register IUserDialogs with the service collection on startup.

This will help encourage designing code with testability in mind, rather than using UserDialogs.Instance, users would be encouraged to resolve IUserDialogs (through class constructor) and call the interface instead. Below is an example of how a view model would change:

From

public partial class FooViewModel
{
    [RelayCommand]
    private async Task ShowAlert()
    {
        await UserDialogs.Instance.ShowAlertAsync("Hello world", "Alerts!");
    }
}

To

public partial class FooViewModel
{
    private readonly IUserDialogs userDialogs;

    public FooViewModel(IUserDialogs userDialogs)
    {
        this.userDialogs = userDialogs;
    }

    [RelayCommand]
    private async Task ShowAlert()
    {
        await userDialogs.ShowAlertAsync("Hello world", "Alerts!");
    }
}

This can be achieved by manually registering IUserDialogs after it has been initialized:

builder.Services.AddTransient((s) => UserDialogs.Instance);

However judging by this SO thread (granted its using the xamarin version), people are unaware this is how you should register the interface for DI and it would be a nice quality of life upgrade for end users!

Changes made in this PR:

  • Removed partial UserDialogs MauiAppBuilderExtension, replaced with MauiAppBuilderExtension shared class (there were 3 files that repeated the same code, DRY refactor + more in line with maui library design patterns).
  • Added transient registration for IUserDialogs in IServiceCollection (Transient reg will mean UserDialogs.Instance is accessed each time this interface is resolved, accessing the latest singleton
  • Updated sample app to use injected IUserDialogs

Open to feedback on these changes, this library is still very Xamarin and changes like this help it become more tailored to Maui! 馃槉

Refactored the MauiAppBuilder extension to be shared by all platforms, add servicecollection registation for instance
Replaced static instance access with injected interface, much cleaner!
@Alex-Dobrynin
Copy link
Owner

Alex-Dobrynin commented Sep 4, 2023

Hello. Cannot you just register UsedDialogs inside MauiProgram.cs like this:
builder.Services.AddSingleton(UserDialogs.Instance);
or one line of code changing something for you?

@Axemasta
Copy link
Contributor Author

Axemasta commented Sep 4, 2023

You can add that line to register, I do it in every maui project referencing this library. Previously with ACR UserDialogs in Xamarin I did the same.

The purpose of this change is to do it automatically for the user because DI is provided by the platform. This is a small quality of life change for the user.

@Alex-Dobrynin
Copy link
Owner

I suppose, user should decide to register or not by himself and I cannot force him to do this. For example, I don't need such functionality and never been.
You may add some boolean to the method, register or not

The host builder can now be updated to indicate whether the IUserdialogs interface can be updated
@Axemasta
Copy link
Contributor Author

Axemasta commented Sep 4, 2023

I have pushed a change to add a new overload to specify whether the interface gets registered. I'm going to raise another branch with a suggestion for refactoring the static singleton.

The whole pattern is very Xamarin and we can reduce the amount of static state in our app by relying on the service collection to store states etc

We could update the registration code to just register the interface and implementation (allowing users to set their own custom type):

public static MauiAppBuilder UseUserDialogs(this MauiAppBuilder builder, Action configure = null)
{
    configure?.Invoke();

    builder.Services.AddSingleton<IUserDialogs, UserDialogsImplementation>();
    builder.Services.AddTransient<IMauiInitializeService, UserDialogsInitializeService>();

    return builder;
}

Use maui initialize service to set the UserDialogs.Instance property:

internal class UserDialogsInitializeService : IMauiInitializeService
{
    public void Initialize(IServiceProvider services)
    {
        var userDialogs = services.GetService<IUserDialogs>();

        UserDialogs.Instance = userDialogs;
    }
}

This means the static instance can now be marked internal, this reduces a code smell that was necessary in Xamarin that we now dont need to workaround in maui

public class UserDialogs
{
    static IUserDialogs _currentInstance;
    public static IUserDialogs Instance
    {
        get
        {
            if (_currentInstance is null)
                throw new ArgumentException("[Controls.UserDialogs.Maui] You must call UseUserDialogs() in your MauiProgram for initialization");

            return _currentInstance;
        }
        internal set => _currentInstance = value;
    }
}

The users registration would be unchanged, but they have the opportunity to set a custom IUserDialogs implementation using a new config class:

public class UserDialogsConfig
{
    private static Type _implementationType = typeof(UserDialogsImplementation);

    public static Type ImplementationType
    {
        get => _implementationType;
        set
        {
            if (!typeof(IUserDialogs).IsAssignableFrom(value))
            {
                // Maybe throw a more specific exception?
                throw new NotSupportedException($"Custom implementations must implement {nameof(IUserDialogs)}");
            }

            _implementationType = value;
        }
    }
}

Usage:

builder
    .UseMauiApp<App>()
    .UseUserDialogs(() =>
    {
        UserDialogsConfig.ImplementationType = typeof(CustomDialogs);
    })

And the host builder extension updates to:

builder.Services.AddSingleton(typeof(IUserDialogs), UserDialogsConfig.ImplementationType);

Now the service collection is responsible for managing the creation and lifecycle of user dialogs which means less static instances floating around leading to better memory performance and less likelihood of memory leaks. etc. Also this means that building code with user dialogs will be more test friendly, encouraging good practises!

@Axemasta
Copy link
Contributor Author

Axemasta commented Sep 4, 2023

You can see my proposed changes for initialization on the latest commit of my feature/di-refactor branch

@Alex-Dobrynin Alex-Dobrynin merged commit 1c2ade8 into Alex-Dobrynin:master Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants