Mvvmcross: Feedback: The new Navigation Service and the life cycle it introduces

[aspnetde]

I spent the day upgrading a relatively new and small project from 4.4.0 to 5.1.1.0. Now I am considering a rollback.

One thing I am stumbling upon is the way the new navigation service is working. Beginning with https://github.com/MvvmCross/MvvmCross/commit/80ef9b903acec0e541259ae058744892876cd405 the navigation service creates new instances of the targeted view models the moment Navigate<T>() is being called.

This is causing me some headaches:

1. The View Model is being created – the constructor is called.
2. The View Model is being initialized by calling Initialize().
3. The View Model is passed to the presenter as part of a MvxViewModelInstanceRequest.

Now let's say we want to pass it to an Android Activity. What we need to to, and what the framework is doing when I didn't miss anything, is serializing the actual View Model at hand and passing it along with the Intent as an extra.

That serialization/deserialization was fine (and necessary, of course) as long as the passed data has been "just meta data" about the View Model which should be used along with the targeted View. But now we have not just meta data in our request, but a whole complex View Model, which may even be doing something the moment we're starting the serialization.

I am not sure about that. But even if I consider it not a problem, I have to make sure that I am passing always the whole full MvxViewModelInstanceRequest to my views, and not accidentaly just a MvxViewModelRequest. If that happens (and it happened to me today, not sure anymore in which cases exactly), the View Model will be created for a second time, when using the IMvxViewModelLoader. And of course it will be initialized for a second time, too.

Which brings me to the second issue: I am not sure about the new Task Initialize(). As this is being awaited the moment the navigation to the view model starts, the whole navigation process stops for as long as that initialization lasts.

I don't know any scenario where I want to have my users experience a frozen UI. What I want my users to see is a view opening the moment they press a button. If initialization takes a while, that's fine – but it has to be running in the background. In the UI we can show some kind of a loading indicator and we can offer the option to cancel, e.g. closing the whole view.

tl;dr:

1. I think creating and initializing the View Model along with the View has been the better approach.
2. I don't see where and why I should use the Task Initialize() for actual long-running initialization work that justifies a Task to be returned.

Thomas

[mvanbeusekom]

Just an informative note, when upgrading to MvvmCross 5.x you can still use the old navigation pattern (using ShowViewModel()). It is not required to switch to using the new navigation service if it doesn't fit your needs.

[martijn00]

Part of the feedback you are giving will be fixed in 5.2.0: https://github.com/MvvmCross/MvvmCross/pull/2072 It is really hard to get all of this right the first time, that is as @mvanbeusekom points out the reason we still have ShowViewModel

[aspnetde]

That's good to know, although I think in our case it's better to stay where we are with 4.4.0 and not to go anywhere until the dust has settled regarding all those rapid changes. See also the other comment I made in https://github.com/MvvmCross/MvvmCross/issues/2107.

[martijn00]

Well, if you stay with 4.4 and you are not trying 5.x we won't know about any bugs you might have either. The rapid changes are a good thing because this means the project is alive. We try to avoid breaking changes and provide backwards compat for a while whenever possible.

[aspnetde]

That's okay from your point of view, unfortunately not from mine. I can't afford to make huge experiments here.

Anyway, I would be happy to hear something about my second point. Any thoughts?

[martijn00]

About your second point. One of the issue's in showviewmodel was that it was fire and forget through reflection, and with that exceptions would be swallowed. With the new implementation they are throw at the invocation point. This makes the whole new Navigation system much better together with the other improvements like returning results, events on changes, deeplinking etc.

[aspnetde]

But what is an Initialize() method worth, when it completely blocks the whole navigation and basically freezes the app while it's running?

[martijn00]

It's not freezing the app, the UI stays responsive. It is awaiting the Initialize on the specific ViewModel. This is to prevent fire and forget like it was before. Also if you want to have the previous behaviour with the new navigationservice you can choose to just not await your navigate, or fire off a Task.Run in Initialize.

[aspnetde]

The navigation process is on hold as long as Initialize() isn't finished. Only after that the request is arriving at the presenter, which is able to present the view. So even if you wrap the whole thing into Task.Run(), which is quite ugly from a API perspective, this won't help you, as the view can only appear after the initialization is done.

So no, there is no fire and forget.

[martijn00]

Did you look at my last PR? (https://github.com/MvvmCross/MvvmCross/pull/2072) This addresses the problems you talk about. In any case, would you say a fire and forget called from reflection is not ugly?

[aspnetde]

Now I found https://github.com/MvvmCross/MvvmCross/issues/2071 which is, I guess, describing what I was experiencing, too. Good to know that this is going to be changed.

In any case, would you say a fire and forget called from reflection is not ugly?

It should not be awaited, that's what I am saying. And there are other ways to do it than reflection.

[aspnetde]

JFYI: I rolled everything back and upgraded again, without using the Navigation Service. The only issue I encountered was the removal of MvxRequestedBy, which we relied on, but that could be easily replaced by passing the sender's information via presentation values.

I hope there will not come a point where the navigation service becomes mandatory, at least not in its current shape.

Th.