matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Extract RoomTileViewModel from RoomTile

Open bwindels opened this issue 4 years ago • 3 comments

First draft of an example how extracting logic from the views into view models can decouple them from the js-sdk, and generally simplify them.

the room property on the view model would ideally not be public, but this would have required to make all the avatar components work without a room, which is a larger task.

The update mechanism isn't perfect yet. Upon re-rendering the parent view, the RoomSublist, it will recreate all the view models. These would ideally live in a RoomSublistViewModel, but again, I wanted to limit the scope of the PR. Recreating the view model while using the same key will swap the view model on an existing RoomTile, so we deal with this in componentDidUpdate by destroying the old view model and attaching to the new one. We'd need to come up with a better way to handle there not yet being a parent view model while doing this conversion one component at a time.

So, if there was a RoomSublistViewModel, it would have a tiles or tileViewModels property which contains all the sub view models. If anything is added or removed from that list, RoomSublistViewModel will emit an update which causes RoomSublist to rerender and pick up the changes in the tiles list.

The concept what can cause re-renders changes somewhat here. Basically components can only trigger re-renders within their own component based on their own view model, but not within their children as is common with React. Each view model emits a change event to trigger a rerender in their component at their level.

Note that some of the boilerplate code in RoomTile could be extracted into a base component class/function specific to work with a view model.


This change is marked as an internal change (Task), so will not be included in the changelog.

bwindels avatar Apr 13 '21 13:04 bwindels

Wrt to how the view model change event triggers a rerender, forceUpdate seemed most straightforward. mobx needs to do something similar, although they have more constraints to track accessed properties on the model. They seem to use a hidden prop that they change to trigger a rerender, because they monkey patch this.props, and that gets reinitialized with forceUpdate, losing the monkey patch.

bwindels avatar Apr 13 '21 13:04 bwindels

Another note is that one can notice there is quite a bit of potential overlap between the stores and the view models (and echo chamber), so even though the view model looks to be only forwarding methods and properties here, it could potentially assume some of those responsibilities.

bwindels avatar Apr 13 '21 13:04 bwindels

Thank you for creating that PR and putting your thoughts in code

I like the approach and decoupling the logic from the views feel really nice. Deriving every single thing from state and creating reusable getters with semantic names is also going to be a huge help in my opinion

We briefly discussed how to author components once the logic is decoupled and from what I can see in this PR there is still a bit of logic mainly there to handle user input and interactions with the DOM. I don't see a clear reason to be so opiniated one way or another for how people write their components leaving the door open to hooks or classes

germain-gg avatar Apr 16 '21 08:04 germain-gg