Shell page dependency resolution optimization

[brunck]

Description

Currently, the Shell page dependency resolution mechanism uses Extensions.DependencyInjection.ActivatorUtilities.GetServiceOrCreateInstance. This allows Shell to create pages without having registered the pages with the service collection, and resolve any registered dependencies that the pages have in their constructor.

This is fairly inflexible, however; the page cannot have multiple constructors that have dependencies that can be satisfied. It may also cause problems when the page's BindingContext is set in XAML, and possibly other issues.

If this was changed to use IServiceProvider.GetService, it would allow for multiple constructors. For example, if you had dependencies registered such as:

// ...
services.AddTransient<MainViewModel>();
// ...

This would allow for a page constructed by Shell to have multiple constructors, with arguments that aren't resolved by the service provider, such as:

public TestPage(MainViewModel viewModel)
{
}

public TestPage(MainViewModel viewModel, OtherDependency arg2)
{
}

The first constructor will be selected by the resolution mechanism.

@PureWeen thoughts?

Public API Changes

The biggest impact is that for any code written using Preview 12, the pages to be constructed by Shell will, going forward, have to be registered with the service collection as part of the HostBuilder. If the dev doesn't do this, once this change was deployed, if they didn't have a parameterless constructor defined, the MissingMethodException would be thrown. This would therefore be a breaking change, but only since Preview 12.

This feature essentially then becomes "opt-in;" for any Shell pages defined with DataTemplate or constructed via Routing navigation, if the pages are not registered, they must have a parameterless constructor, as they will be created by Activator.CreateInstance. If they are registered, they will be resolved by IServiceProvider.GetService(), and any of the registeed dependencies defined in their constructor will be resolved.

Intended Use-Case

This allows Shell to create pages without dependency resolution, and the pages can be constructed with dependency resolution if desired. It also allows multiple constructors to be defined, allowing more flexibility for the developer.

[eerhardt]

This is fairly inflexible, however; the page cannot have multiple constructors that have dependencies that can be satisfied

Can you explain this inflexibility a bit more? ActivatorUtilities.GetServiceOrCreateInstance allows for multiple constructors - it picks the longest one that it is able to satisfy.

It may also help to have a small repro app showing the behavior you are not expecting.

[brunck]

You're right; after a test I see that it does now work with multiple applicable constructors. My research found that this wasn't always the case, it used to throw an InvalidOperationException. This must have been fixed at some point.

[PureWeen]

@brunck can we close this issue and the related PR?

[brunck]

@PureWeen well, what do you think about the "opt-in" aspect, which would allow people to opt-out by not registering the page classes? Not being able to opt-out seems to have caused some problems for people.

[PureWeen]

related

#4318

[PureWeen]

@brunck if you can provide a repro of the problems people are seeing that would be useful. I posted a request for that on a related issue as well. The only problem I can come up with is if you had multiple constructors and you always want shell to pick the default ctor and never the constructor that has parameters. But it seems like people are hitting a different issue that I'm not having success at reproducing

[brunck]

@PureWeen I added a test case to the PR that reproduces a problem described in the other issue, specifically where there is a parameterless constructor as well as a parametered constructor, and nothing is registered (no page classes and none of any classes used as parameters in the 2nd constructor).
This blows up using the current code because it picks the parametered constructor and the system can't resolve its parameters, but using the PR code it just uses the parameterless constructor.
I'll see if I can repro anything else, otherwise I guess we'll have to wait and see if something shows up on the related issue.

[PureWeen]

@brunck so it looks like this is this is the reason why we're all seeing a bit different results when testing this one out.

#4318 (comment)

So, until the issue referenced in that comment is fixed your approach might be the best.

[jonathanpeppers]

When I tested out dotnet/maui/maui today, that has 815ea34.

I hit crashes on startup because this page has no default ctor:

https://github.com/microsoft/dotnet-podcasts/blob/962b15c578bb5748e3d5f530d1b27e2fe24d812b/src/Mobile/Pages/DiscoverPage.xaml.cs#L7

I had to add this:

builder.Services.AddTransient<CategoriesPage>();
builder.Services.AddTransient<CategoryPage>();
builder.Services.AddTransient<DiscoverPage>();
builder.Services.AddTransient<EpisodeDetailPage>();
builder.Services.AddTransient<ListenLaterPage>();
builder.Services.AddTransient<ListenTogetherPage>();
builder.Services.AddTransient<SettingsPage>();
builder.Services.AddTransient<ShowDetailPage>();
builder.Services.AddTransient<SubscriptionsPage>();

@PureWeen do we need to fix this? file an issue?

[brunck]

@jonathanpeppers this is specified above, in the description. The pages to be resolved by Shell with DI have to now be registered, otherwise they have to have a default constructor.

[jonathanpeppers]

@brunck right we need to at least document this in release notes or something?

I was using MAUI Preview 13, then switching to main, the app started crashing.

[brunck]

@jonathanpeppers yeah, I don't know that I'm authorized to do that/where best to do it, otherwise I'm happy to do what is needed.

That makes sense; I believe this code change is in main but not in the P13 branch.

[jonathanpeppers]

Yeah, I'm relying on MAUI team to know what to do, thanks.