reactiveui/Sextant

feature: Consolidate Extension Methods for Navigation Registration

RLittlesII opened this issue · 5 comments

Is your feature request related to a problem? Please describe.

Currently there are two methods required to register stuff for navigation against Splat. Looking at it after the fact, this was a bad decision on my part.

RegisterView<MainPage, MainViewModel>();
RegisterViewModel<MainViewModel>();

Describe the solution you'd like

Simplify this into a single method that allows for a few overloads.

RegisterToNavigate<TView, TViewModel>()
where TView : IViewFor
where TViewModel : class, IViewModel

Then provide overloads for registering complex objects as Splat does not support constructor injection.

Relates To: #230

Describe alternatives you've considered

Nothing, this is an attempt to reduce the cognitive load of a developer using the library. Give them one point to register everything needed for navigation.

Describe suggestions on how to achieve the feature

Additional context

I like that change, it indeed confused me initially.

Would it also be possible to rename the IViewModel interface? IMO the interface should denote that the implementing view model participates in routing. In @kentcb's original implementation he deliberately distinguished between IModalViewModel and IPageViewModel which is more readable IMO. A unified interface also is fine, as long as there is more distinction. Here some ideas:

  • ISextantViewModel
  • IViewStackViewModel
  • IRoutableViewModel, taken by ReactiveUI
  • IRoutingViewModel, maybe too easy to confuse with IRoutableViewModel?

Additionally, I don't see a reason for the new() constraint on RegisterViewModel<>() method.

ReactiveUI recommends the following pattern for dependency resolution which allows for good unit testing:

public HomeViewModel(AuthenticationService authenticationService = null)
{
    _authenticationService = authenticationService ?? Locator.Current.GetService<AuthenticationService>();
}

The pattern violates the new() constraint on RegisterViewModel<>(), though.

rms81 commented

Is it ok if I start looking into this one?

@schnerring You are correct on several counts. The original did have two interfaces. When we ported it we didn't want a marker interface to be required if your plan was to display a page modally or not.

For the v3 API I have been considering ISextantViewModel for the base extension point.

That is a good point. The new() constraint was probably an oversight. It's intention was to force a segragation and could easily be reworked.

There will most likely need to be some form of a RegisterForNavigation<TView>(Func<TViewModel>) as folks who prefer to locate their ViewModels themselves will have to provide a delegate for registration.

@rms81 Yes. This is a good first time story. If you want to get started on it, feel free. If you need help reach out.

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