microsoft / TemplateStudio

Template Studio accelerates the creation of new WinUI 3, WPF, and UWP apps using a wizard-based experience.
Other
2.68k stars 459 forks source link

VMs are tightly coupled to the UI layer #2867

Open ericgrover opened 5 years ago

ericgrover commented 5 years ago

For new Features:

There are references to Windows.UI in the VMs that glue the VMs to the UI. It would be better to use dependency injection to inject the Navigation Service into the VMs and for the VM to code against an interface for navigation.

It would also be better to not use the XAML Visibility type in the ViewModels, but instead use boolean properties and use a VisibilityConverter in the XAML data binding.

By decoupling the UI layer from the VMs, the VMs can be more easily adapted to work with Xamarin in the future.

crutkas commented 5 years ago

great feedback @ericgrover

mrlacey commented 5 years ago

The coupling between the Views and ViewModels is something that has been discussed many times previously.

Reasons the generated code includes the level of coupling it does include (but are not limited to)

Ultimately, it's something I'd like to see added, but it's a very low priority.

Some follow-up questions:

ericgrover commented 5 years ago

Matt,

Thanks so much for your quick and detailed reply.

Here are my responses to your questions:

• How much of an issue is this? o After creating a new project, I would remediate these architectural issues before going any further in development. • Are there lots of people building UWP apps and then trying to reuse ViewModels when they build a Xamarin (or other) app? (Anecdotal evidence says no, but if there are people planning this we'd love to hear from them/you.) o We are about to begin building a UWP app for a client that is currently targeting Win 10 tablets, but can easily anticipate a 2nd or 3rd phase that would target iPad or Android tablets.
o In addition to support for Xamarin Forms, decoupling the UI layer from the VMs also allows for easier unit testing. You could easily create a mock navigation service to inject for testing purposes. • Do you think it would be beneficial to have a separate option within the generation process to specify having decoupled Views and ViewModels? o Probably. This has always been a problem with Microsoft’s new project templates and sample code. In order to keep things accessible, you end up creating tools that get you 80% of the way to production quality code, but never quite there.
• What else would you want to be changed if the View and ViewModel were decoupled? o The logic for hiding the keyboard accelerator tooltips really doesn’t belong in the view model. It’s UI code that belongs in the code behind. MVVM does not prohibit putting code in a code behind if it is code that manipulates UI elements (rather than providing a model for the view). UI code belongs in the UI layer.

mrlacey commented 5 years ago

tools that get you 80% of the way to production quality code, but never quite there.

Sadly, everyone I've spoken to about WTS not quite giving them exactly what they want has very different requirements for the remaining 20%.

That WTS is still considerably more valuable than just starting from a blank project definitely makes it useful. However, it's good to be reminded that there are more experienced developers who have different requirements and expectations.

ericgrover commented 5 years ago

Jerry Nixon's Template10 project is a very nice alternative. Gives you a very similar bootstrapping capbability with a better MVVM implementation. Unfortunately, he doesn't have the rich implementation of all the Win 10 features that this tool does. https://github.com/Windows-XAML/Template10

mrlacey commented 5 years ago

@ericgrover for information: template10 is merging with the Prism project as their UWP implementation. When that's complete we expect to update the WTS templates accordingly.

Kaffiend commented 5 years ago

@mrlacey It looks like the Template10 approach didn't work out for the prism team. see UWP Platform Support #1745

mrlacey commented 5 years ago

@mrlacey It looks like the Template10 approach didn't work out for the prism team. see UWP Platform Support #1745

What the future holds for existing UWP Prism users isn't clear and little has, so far, been said publicly about this so I don't want to speculate. Also, AFAIK, the Template10 team haven't yet commented on what their plans are now. For now, the best we can do is wait and see.

mrlacey commented 3 years ago

Pinging this in relation to WinUI3 templates. I asked this question there and it was ignored.

Has any consideration been given to reducing the coupling of ViewModels to views with these new templates?

I've noticed developers are increasingly putting their ViewModels in a .NET Standard library to enforce a reduced coupling of the ViewModel to the UI

vmadurga commented 3 years ago

Hi Matt, thanks for bringing this up again.

We agree to decouple the ViewModels to the Views, we will open a new issue to review and adapt out current WinUI 3 templates.

mikebattista commented 2 years ago

Closing since the linked issue that was created was completed.

mrlacey commented 2 years ago

Looking at the current implementation (via the linked PR) there is still some tight coupling of the VMs to the UI and elsewhere. Looking particularly at: ShellViewModel, SettingsViewModel, and WebViewViewModel.

While the WinUI3 code is definitely better in this aspect than the UWP code, it still has some noticeable issues.

How much of an issue this is, is open to debate. Here are some important questions to answer. (I'll save my answer until later to so try and avoid leading others.)

Reopening to encourage further discussion.