Windows-XAML / Template10

Making Windows 10 apps great again
Apache License 2.0
1.41k stars 389 forks source link

Reconsider if ViewModelBase should implement NavigationService #1721

Open JerryNixon opened 4 years ago

JerryNixon commented 4 years ago

It's already possible to retrieve the NavigationService in a Template 10 view-model using the GetNavigationService() extension method off the NavigationParameters passed into the OnNavigatedTo and OnNavigatedToAsync methods. You can also use GetSynchronizationContext() to help with threading. These are not discoverable, I know, but they are easy.

Here's the syntax:

public class MyViewModel : ViewModelBase
{
    private INavigationService _nav;

    public override void OnNavigatedTo(INavigationParameters parameters)
    {
        _nav = parameters.GetNavigationService();
    }
}

The issue here, however, is the change in heart I have had with NavigationService. I believe that an app should create its own NavigationService, lately I have been calling mine NavigationManager in order to disambiguate the name. The reason to remove it from ViewModelBase as a public property is that it encourages a generic use case instead of an app-specific one. It also shows up when using binding and that's really confusing.

Here's what I am recommending:

public class NavigationManager
{
    private readonly NavigationService _navigationService;

    public NavigationManager(NavigationService navigationService)
    {
        _navigationService = navigationService;
    }
}

public class MyViewModel : ViewModelBase
{
    private readonly NavigationManager _nav;

    public MyViewModel(NavigationManager navigationManager)
    {
        _nav = navigationManager;
    }
}

This is not a recommendation to remove the extension methods or the ability to access the NavigationService. Just removing the public property on the view-model in order to encourage better patterns. If you don't want better patterns, then the extension methods are still there. You can also sub-class ViewModelBase in order to restore the property, too.

panoukos41 commented 4 years ago

I think this is the right way to proceed, that way people can always implement their own services if they wish to without worrying for services they don't need.