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

[Shell] Application crashes Page constructor arg cannot resolve type from service provider (wrong constructor used) #4318

Closed
PedroCigaw opened this issue Jan 25, 2022 · 6 comments · Fixed by #4281
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 t/bug Something isn't working

Comments

@PedroCigaw
Copy link

PedroCigaw commented Jan 25, 2022

Description

The new feature of constructor injection on shell routes is great, except they break my code and is inconsistent with DI behavior.

Essentially my code stopped working as I have existing constructor overloads I use for other purposes, now my application crashes when the router is creating the view. This is because the new implementation uses Extensions.DependencyInjection.ActivatorUtilities.GetServiceOrCreateInstance and the issues is if the type is not registered the extension method then will try and create the instance using the ConstructorMatcher and ultimately breaks my code as it is now changing the default and expected behavior. i.e. previously Activator.CreateInstance

Personally I think types should be explicitly registered to the container and just as I would have to register a ViewModel registering the View isn't a big deal and would only need to be done if needed, its nice and clear and more importantly consistent. What I don't expect is to have to create registrations or code to bypass this sort of feature.

Steps to Reproduce

Create Maui App
Create a Shell
Create a Page and add a new Constructor with a type that is not registered to the container.
Add the page to the shell as a route
Run

Version with bug

Preview 12 (current)

Last version that worked well

Preview 11

Affected platforms

iOS, Android, Windows, macOS

Affected platform versions

iOS 15, Android, Windows

Did you find any workaround?

Register the View to the container with a factory method services.AddTransient(sp => new TView())

Relevant log output

No response

@PedroCigaw PedroCigaw added the t/bug Something isn't working label Jan 25, 2022
@jsuarezruiz jsuarezruiz added area-controls-shell Shell Navigation, Routes, Tabs, Flyout fatal labels Jan 25, 2022
@eerhardt
Copy link
Member

Would marking the constructor you want to be used in DI with ActivatorUtilitiesConstructorAttribute help here?

@PedroCigaw
Copy link
Author

Yeah I did consider that and it is another possible work around. The reason I avoided it is I didn't want to litter the code base with attributes. I preferred the approach of registering in DI with factory a method, that way if this feature changes which I hope it does I only have remove the code in one place and happily register the Views I do want include, well I've all ready started doing that ;)

@PureWeen
Copy link
Member

PureWeen commented Jan 27, 2022

@PedroCigaw I think I don't quite understand your repro steps

I did all your steps and added an extra constructor to MainPage

	public MainPage()
	{
		InitializeComponent();
	}

	public MainPage(StringBuilder builder)
	{

	}

And it resolves to the default ctor for me with no exception. Can you attach your repro please?

@PedroCigaw
Copy link
Author

I have created a repository on github to illustrate the problem https://github.com/PedroCigaw/SampleIRouteInjection

If you look at the LoginView there are two constructors if you run the app you will get an exception as in my view the wrong constructor is being called as a result of the internal implementation of the Router.

To get past this issue I have to

  1. register the RandomItem class to DI which I don't want to do as that presents other problem or
  2. I have to register the view with a factory method to create it. (as per the commented line in MauiProgam.cs
  3. I could also decorate the constructor to use by using ActivatorUtilitiesConstructorAttribute

The behavior I would expect, if I wanted dependency injection on the View then I would register it to DI just like everything else. It just feels wrong to have to do a registration to avoid a internal behavior just because in this case its being resolved by the Router.

Hope that helps please let me know if you have any issues or questions on the sample.

@eerhardt
Copy link
Member

Can you try flipping your constructors around and see if that helps?

    public LoginView()
    {
        InitializeComponent();   
    }

    public LoginView(RandomItem item)
    {
        //this is constructor is invoked by code and RandomItem is not registered in DI

        InitializeComponent();
    }

You may be running into: dotnet/runtime#46132

@PedroCigaw
Copy link
Author

PedroCigaw commented Jan 28, 2022

Thanks @eerhardt interestingly that does work, but I wouldn't call that a solution to the problem, where I now have to worry about the order of constructors in the code base, but rather its another work around imo.

BTW dotnet/runtime#46132 if the internal implementation changes as a result of this issue then this work round my not work in the future.

The point I am highlighting is the approach is flawed and inconsistent with DI and .net. It is much easier to document and explain to developers that if you want DI resolution of your Views when using routing just register the View to DI along with all your other registrations., simple clear and consistent.

I now use a little helper method that just gets all the Views in a namespace and registers them using factory method to construct them, no need to go changing constructor order, use attributes etc to get past the problem and this works fine, but my point still remains.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout platform/android 🤖 platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst platform/windows 🪟 t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants