DryIoc

[joacar]

This PR contains a FormsApplication class that, when built in Test configuration, acts base class for PrismApplicationBase instead of Xamarin.Forms.Application to let us test properly.

As new configuration is introduced the appveyor config must be updated to encompas this.
#517

[dnfclas]

Hi @joacar, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[brianlagunas]

I see this is where you added the Test configuration that PR #412 depends on. This actually looks really good. I don't think having the Test config is a big deal, and it adds a ton of test coverage. It seems my abstraction of the IApplicationProvider caused some conflicts. Do you want to fix them, or do you want me to just copy and paste what you have here into my solution?

[joacar]

Feel free to decide what is most convenient for you. I'm happy to do it tomorrow when I've time.

I'll create a new branch from master and implement the Test-configuration approach there and then submit a PR. Is that OK?

Is there an appveyor config to include the new configuration? Currently I think that the Test configuration is based on Debug, I'll change it to Release

[brianlagunas]

Yes, please update and change the configuration to be based off of Release. I'll update AppVeyor to build off the Test configuration.

[brianlagunas]

Can you get this one sync'd up with master?

[joacar]

Synced and merged.

[brianlagunas]

The navigation service cannot be a singleton. We have to create a new instance each time and then override the parameters of the VM ctor to inject the correct instance. Can this be done in DryIoc?

[joacar]

Saw this as well yesterday and posted a question on stackoverflow since I couldn't find how to do it in the docs.

DryIoc supports resolving overrloads using func var @override = Container.Resolve<Func<INavigationService, TypeToResolve>>(); return @override(navigationService) however this fails since TypeToResolve is not known at compile time.

I'll dig into this!

[joacar]

Fixed. Need to update some tests as well as a new test to check the added functionality. Hopefully I will have time during the day but right now my job calls for attention.

[brianlagunas]

We have moved to using a named instance for extensibility reasons. Yu can see the Unity registration here: https://github.com/PrismLibrary/Prism/blob/master/Source/Xamarin/Prism.Unity.Forms/PrismApplication.cs#L82

And the usage here: https://github.com/PrismLibrary/Prism/blob/master/Source/Xamarin/Prism.Unity.Forms/PrismApplication.cs#L51

This allows the developer to provide their own implementation more easily by overriding the INavigationService registration wit their own implementation.

Can we do it like this?

[joacar]

One option is to use named registration Container.Register<INavigationService, DryIocNavigationService>(serviceKey: "dryIocNavigationService"); and resolve using Container.Resolve(serviceKey: "dryIocNavigationService");

Another option availble using DryIoc is Container.Register<INavigationService, DryIocNavigationService>(ifAlreadyRegistered: IfAlreadyRegistered.Replace); that simple replace the previos registration.

Not sure if one has any advantage over the other (except that the last clears all previos registrations). Will ask the creator of DryIoc which is most performant

[brianlagunas]

Let's use the named instance and go with consistency with the other containers. This makes documentation a lot easier too. Whenever I get to that point :)

[dansiegel]

@joacar if you need to make it internal you can add

[assembly: InternalsVisibleTo( "Prism.Forms.Tests" )]

[joacar]

@dansiegel Tried that first time but it didn't compile. But that was because I forget to add the internal modifier to the method.. Stupid stupid me.

Thanks

[brianlagunas]

I don't know... this feels a little dirty to me. What if we just created a method responsible for creating the navigation service for the page instead of doing that logic inside of the ConfigureViewModel method directly? Maybe something like

INavigationService CreateNavigationServiceForPage(Page page)
{
    var navigationService = CreateNavigationService(); 
    ((IPageAware)navigationService).Page = page; 
    return navigationService;
}

Thoughts?

[joacar]

I like that - it makes it clear that the navigation service utilize the page instance. Also I was thinking refactoring ConfigureViewModel like this

protected override void ConfigureViewModelLocator()
        {
            ViewModelLocationProvider.SetDefaultViewModelFactory((view, type) =>
            {
                var page = view as Page;
                if (page != null)
                {
                    return PageViewModelFactory(page, type);
                }
                return Container.Resolve(type);
            });
        }

        /// <summary>
        /// Resolve the view model of <paramref name="type"/> associated with <paramref name="page"/>
        /// </summary>
        /// <remarks>
        /// The method will set the <see cref="IPageAware.Page" /> property on the <see cref="INavigationService"/>  
        /// instance that will be injected into the view model.
        /// </remarks>
        /// <param name="page">The <see cref="Page"/> associated with the view model</param>
        /// <param name="type">View model type to resolve</param>
        /// <returns>View model instance of type <paramref name="type"/></returns>
        protected virtual object PageViewModelFactory(Page page, Type type)
        {
            // Use CreateNavigationServiceForPage here instead
            var navigationService = CreateNavigationService();
            ((IPageAware)navigationService).Page = page;
            ResolveTypeForPage(page, type, navigationService);
            // Resolve type using the instance navigationService
            var resolver = Container.Resolve<Func<INavigationService, object>>(type);
            return resolver(navigationService);
        }

Moving back to the original question, yes, it feels dirty. That method exists soley for the purpose of grabbing the instance injected into the view model so that it can be tested that the Page property of IPageAware is correct.

Another option would be to add extension methods (for test only) to INavigationService that returns the view model navigated to. E.g. Task<TViewModel> NavigateTo<TViewModel>(). Then we can check that the navigation service in the mock view model is correctly initialized.

If you have a better way please share

[brianlagunas]

What if the test didn't involve testing the method directly, but rather we just test the result: creating a Page, then get the VM from the binding context (which has a public navigation service property, grab the nav service, and cast it as IPageAware, and then make the check. Instead of trying to so tricks with methods and assembly attributes.

[joacar]

Perfect. Feel so stupid I didn't think of that before. Will implement the changes and adjust the tests.

Thanks

[brianlagunas]

I don't see where the IPageAware is being set

[joacar]

It is set in CreateNavigationService(Page)

[brianlagunas]

I made some changes to the PrismApplicationBase which will make it easier to create container projects in the future and make them easier to maintain. Could you rebase from master and update the PrismApplication? We're almost there :)

[joacar]

Yes I'm aware of the changes, just havn't hade time to fix them :) Will implement the changes later this afternoon or tomorrow!

[brianlagunas]

This should be Name, not FullName

[joacar]

Line 83 in PageNavigationService use typeof(ViewModel).FullName. Shouldn't this be using Name as well, otherwise it won't work

[dansiegel]

@joacar you can look at the updated Unity version which was updated for more clarity. It's not about Pages and classes. Since Prism is about MVVM it's really about Views and ViewModels which with Xamarin Forms are fulfilled with Pages and BindableBase types. You navigate to View's not ViewModel's which is why for TView we just get the type Name, and to discourage the bad practice of navigating to a ViewModel we're taking the FullName of the TViewModel.

[joacar]

Thanks for pointing me in the right direction.

[brianlagunas]

It's missing the new IPlatformInitializer implementation. See the Unity project for how to implement.

[brianlagunas]

do we want to assert anything in this test?

[ederbond]

Congratulations @joacar
I've never used DryIoC but after a few research I realized this is one of the fastest IoC Frameworks nowadays. Definately will use this with prism after your PR will be aceepted.

@brianlagunas Is there a timeframe for the release of Prism.Forms 6.2.0 + Prism.DryIoC.Forms aut of pre ?

[brianlagunas]

@ederbond waiting on Xamarin to release the stable version of XF

[brianlagunas]

Great PR! Thanks for the hard work!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.