MVVM - The MemberList example

[langleyd]

The purpose of this PR is to:

• demonstrate practically how we could use MVVM
• highlight the benefits it brings and some suggestions for best-practice
• gather feedback on whether this is a useful direction to bring the codebase

The PR is large as it's difficult to demonstrate an architecture and the benefits without a pretty complete example. It's functioning but not complete(there are more improvements to make and more things to test). If we are happy to move ahead with this as an approach we would apply it as a series of separate PRs(We do actually want to make UX improvements to the MemberList that are not included int he scope of this example).

MVVM

Summary

The Model-View-ViewModel (MVVM) pattern helps cleanly separate an application's business and presentation logic from its user interface (UI). Maintaining a clean separation between application logic and the UI helps address numerous development issues and makes an application easier to test, maintain, and evolve. It can also significantly improve code re-use opportunities and allows developers and UI designers to collaborate more easily when developing their respective parts of an app.
Using the MVVM pattern, the UI of the app and the underlying presentation and business logic are separated into three separate classes: the view, which encapsulates the UI and UI logic; the view model, which encapsulates presentation logic and state; and the model, which encapsulates the app's business logic and data.

Better separation of concerns

• Problem In many cases, as with the member list, our datamodel(via the js-sdk) and the presentation logic are implemented together directly in the view component(MemberList.tsx). This means this logic has to be understood and tested all together as one unit.
• Solution: These layers are kept separate via MVVM. In the updated code:
  • MemberService wraps our business logic(from the js-sdk) and returns the data model which is make up of simple data types. In this PR I've added to some models that already existed under models/.
  • MemberList contains just UI code(Functional Component) . It takes only simple data types and functions to send back commands.
  • MemberListViewModel contains just presentation logic and state.
• Benefit Each layer can be tested individually. E.g. In this PR I have created "MemberListViewModel-test.ts" and created a Mocked implementation of the IMemberService interface so that for the defined data model inputs and users commants we assert the presentation logic works by checking the model outputted for the UI. Similarly this also makes it easier to test the UI. We could again use the the Mocked Service and the ViewModel to output the different valid states for a particular screen, then rendering output and asserting it's validity using snapshot/screenshot component tests. Screentshot tests like this should be predictable(i.e. for this data the ui should look like this). E.g. Maybe in removing js-sdk from the avatar UI components, by separating business and presentation logic from the UI we can isolate some of the bugs that are causing tests flakiness.
• Benefit Each layer can be understood separately and therefore helping us keep a better handle on complexity. Is something not rendering correctly? Go check the view code and the simple data inputs. Is it a problem with presentation logic? Go write a unit test to test a hypothesis that a particular combination of user commands and data inputs is caucusing an invalid output state.
• Benefit The js-sdk is no longer referenced from the view components, nor is it referenced from the ViewModel. This means the views and ViewModels can be re-used (E.g Re-use this memberlist in aurora, or re-use the timeline audit-bot in the admin interface). Or if we want the feature to work with another SDK like matrix-rust-sdk we just create another concrete implementation of IMemberListService to pass to the ViewModel.
• Benefit It could help bring structure and a shared understanding of how we build features. There are prboably plenty of ways we could make the code bellow better. Maybe we could iterate on member list with other similar improvements to have a reference implementation that we consider best practice(E.g. enable axe tests).

Related Material

Uncle bob's clean architecture is a complementary resource on how to organise a codebase, it's come up in conversation so just linking here in case anybody is not familiar or wants a refresh.

[t3chguy]

Our code style says not to use I-prefixes on interfaces

[t3chguy]

Why can't this load/unload be done by the useMemberListViewModel hook? or at least have a useModel(viewModel) hook.

[t3chguy]

doc comments needed here, what's the diff between load and loadMembers given one implies this concerns only members based on its name

[langleyd]

Yea some poor naming clashes there, will update and document properly. load/unload were meant as general ViewModel lifecycle functions, loadMembers is a relic of the original implementation.

[robintown]

Hi, since I've been transitioning Element Call to an MVVM architecture recently I thought I'd provide some feedback. This initiative is really cool to see, but in my view there are also some missed opportunities here:

• Being framework-agnostic. Traditionally a view model is completely independent of whatever UI framework the application uses, which lends itself toward being a plain class which holds UI state in some sort of reactive primitive (such as observables or flows). Here I see MemberListViewModel is actually a React hook. Maybe it's true that we're unlikely to ever move away from React? But working with primitives can make testing easier, and it's generally a nice feeling to know that your project is free to evolve towards new frameworks over the long-term.
• Embracing functional reactive programming (which is about "specifying the dynamic behavior of a value completely at the time of declaration"). This isn't necessarily a part of MVVM, but modern reactive primitives make it really easy to program in this style, and it can make code a lot less error-prone when you're trying to produce, update, or respond to dynamic values. React is good at the simple stuff like mapping one dynamic value to another or zipping several values together, but with a primitive like observables, you get access to a whole new world of operators for silencing updates, accumulating values, timing out, etc. in a highly composable fashion.

I imagine there are benefits to keeping everything contained inside React, like familiarity and integration with existing hooks, but I do want to at least share how pleasant the experience with observables has been in Element Call! For instance, here is an observable deciding which speaker to put in the spotlight. Previously this logic was spread out across multiple files, and you had to follow the dynamics of some React components to understand it. Then to bind those observables to React, we're just calling hooks from the observable-hooks package.

Hope my perspective is useful in some way 👋

[langleyd]

Thanks for your feedback @robintown !

React/Web isn't traditionally my wheelhouse(I come from mobile and it's been a few years since i've written React).

What led me toward using hooks for the viewModel was just that it seemed like a React-y way to combine logic and state.

Having continued to play with this example, I've tied myself in knots a little trying to get my use callbacks/effects/dependencies correct and it worries me a bit what it would be like with a more complicated ViewModel. This might just be due to my inexperience, but I can't help but think it would be simpler with a plain class. As you said this would be more the norm for ViewModels, so I think re-working with a class might be a good next steps for this.

I've used observables plenty coming from mobile to similar effect, with the ViewModels presentation state. It's also something that has tended to divide opinion on the teams I've worked in the past so I'd be interested to get a sense of how others feel.

[dbkr]

Thanks so much for doing this. It's really illustrative of how the view the split can look. Yeah, having the view model as a hook will obviously be limiting if we do want to switch frameworks, and the hook logic can be hard to get your head around. That said, it could save a lot of boilerplate code since the hooks can automatically integrate seamlessly with the components (eg. setup/teardown on mount/unmount).

How would we test the View itself? I'm wondering about the viewmodel being passed in as a prop to the view so that tests could do this. It would make it more awkward to use though.

[langleyd]

I have changed to a ViewModel written as a class and that uses rxjs to represent the presentation state. As @robintown suggested I found this much easier to handle the flow of data and it was way quicker to write and debug.

The viewModel is still exposed as a hook and I used observable-hooks as suggested. I kept all of the observable-hooks usage to the hook and this avoids any usage of observables spreading to the React component itself. The viewmodel is still exposed to the component as a combination of simple state and callbacks.

@dbkr as it is just a simple interface exposed we could find a way to test the view by providing a mock implementaiton of that hook interface. Or as we do on mobile, test the and viewmodel as unit together and then I think you only need to use the MockMemberService which is already mocked for the unit tests of the ViewModel. Will try add some component tests shortly.

[dbkr]

Yeah, the little layer of glue between rxjs and react seems okay and avoids complete dependency on react. Testing the two together might be easiest if we can just mock out the service, although I assume we want to split them out sometimes as that's part of the advantage of MVVM. As long as we can mock the viewmodel though, that's fine.

[MidhunSureshR]

Some thoughts and questions:

[MidhunSureshR]

I'd opt for dependency injection here i.e you pass the vm as prop. Otherwise how would you render this component with a mock/different vm?

[langleyd]

Yup, makes sense thanks. Was going to do something like this next so I could create component tests.

[MidhunSureshR]

The view should only depend on the view model so these should be moved to the vm.

[langleyd]

For callbacks specifically that are only relevant to the parent component (so if we are forwarding to the VM it will probably just be proxied to the parent) I think maybe it's open to interpretation of the MVVM implementation, I've seen done both ways. Might depend on the design of the overall navigation. I'm happy to go with whatever we align on though.

[MidhunSureshR]

I'm not convinced that rx-js provides any value here. Data from the MemberService is first converted into an observable and then immediately converted into a react state. All this complexity is worth it if we do want to move away from react but otherwise YAGNI. Also remember that this glue needs to be repeated for each view.

[langleyd]

rx-js was used here to ease the implementation of the ViewModel by providing a reactive presentation state(so that the UI updates when the presentation state does). The view models's role is generally to transform data and notify downstream of it's updates, which tends to fit well reactive libraries like this.

So it's not an attempt to move away from react, just a pragmatic decision to simplify the implementation of the ViewModel( I found it much easier to implement with like this than with a hook).

Have you an alternative in mind @MidhunSureshR?

[MidhunSureshR]

I imagine eventually we'd refactor MemberTile to also use MVVM. How would the code look then?
I imagine :

return <MemberTile viewmodel={?} />;

Where would the view model come from in this case?

[langleyd]

I'm guessing you're considering this as the member tile contains things like the shield, requiring dynamic e2e status?

Two approaches I can think of:

1. Nested view models
2. Just consider it another "non-smart" functional component and it's the responsibility of the MemberListView to provide the e2e data. If we found a need to re-use that state across view models, then use some form of composition.