Extract RoomTileViewModel from RoomTile #5857

bwindels · 2021-04-13T13:01:32Z

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.

although while testing, the view_room never seems to get dispatched? so it looks like this might be dead code anyway, still porting it though

bwindels · 2021-04-13T13:13:12Z

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 · 2021-04-13T13:17:30Z

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.

germain-gg · 2021-04-14T10:32:41Z

src/components/views/rooms/RoomTile.tsx

@@ -384,25 +239,25 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
                <IconizedContextMenuOptionList first>
                    <IconizedContextMenuRadio
                        label={_t("Use default")}
-                        active={state === ALL_MESSAGES}
+                        active={volume === ALL_MESSAGES}


Should all of these be moved to be getter in the model?

Yeah, can do. We'll also need a setter for each.

germain-gg · 2021-04-15T09:42:27Z

src/components/views/rooms/RoomSublist.tsx

@@ -522,11 +523,13 @@ export default class RoomSublist extends React.Component<IProps, IState> {
            const visibleRooms = this.state.rooms.slice(0, this.numVisibleTiles);
            for (const room of visibleRooms) {
                tiles.push(<RoomTile
-                    room={room}
+                    model={new RoomTileViewModel({


That might be personal preferences, but I believe spreading the enumerable properties of the view model onto the component might be better

React only does shallow comparison on props for things like React.memo. Which would force us to implement a areEqual function every time
It also reads a bit better when going through the debugger sidebar for example

The same goes for the props comparison with React.PureComponent. We'd have to implement shouldComponentUpdate

Not quite sure what my preference would be yet, but about shouldComponentUpdate, I believe you'd need either way when there are complex nested objects inside the model, such as room.

Yeah, spreading the view model properties was also my first though. I haven't come up with a good way to trigger the component to rerender though this way. The parent would have to rerender basically when a child view model emits change. And what triggers the parent to rerender if not forceUpdate or some state variable? Might be better to just put that in the child component then and not also rerender all the sibling components. forceUpdate has the advantage of not needing to implement shouldComponentUpdate...

I'd be happy to learn about other possibilities here though

I actually went with spreading the view model properties over the component state (so it can update itself by listening to the view model)

germain-gg · 2021-04-16T08:10:17Z

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

turt2live

Overall it seems sane - just a couple nitpicks about classes and representation. Docs would also be good :)

turt2live · 2021-04-16T20:27:00Z

src/components/views/rooms/RoomTile.tsx

+    public componentWillUnmount() {
+        this.props.model.off("change", this.rerender);
+        this.props.model.off("ensureVisible", this.scrollIntoView);
+        this.props.model.destroy();


it's a little surprising that the model gets destroyed here, but I suppose it's the right thing given not much else will know when is best to destroy it...

Indeed, in a larger example, the parent view model would destroy the child view model when needed (e.g. upon navigation, ...), and the child view model being cleared would cause a re-render and the component to unmount. Given there is no parent view model here yet, I reversed things.

turt2live · 2021-04-16T20:28:26Z

src/components/views/rooms/RoomTile.tsx

+        this.props.model.off("change", this.rerender);
+        this.props.model.off("ensureVisible", this.scrollIntoView);


we could probably make an abstract class which does a lot of this lifecycle management, making the components smaller/easier to write. Could even make the class take a T to make this.props.model magically work.

Have tried to do this fwiw

jryans

Overall, the core design pattern here makes sense to me, and seems like a great path forward, thanks for working on this example! 😄

I agree with @turt2live that it would be great to have some longer docs explaining the pattern and some of the design nuances.

jryans · 2021-04-22T15:07:41Z

src/components/views/rooms/RoomSublist.tsx

@@ -522,11 +523,13 @@ export default class RoomSublist extends React.Component<IProps, IState> {
            const visibleRooms = this.state.rooms.slice(0, this.numVisibleTiles);
            for (const room of visibleRooms) {
                tiles.push(<RoomTile
-                    room={room}
+                    model={new RoomTileViewModel({


Not quite sure what my preference would be yet, but about shouldComponentUpdate, I believe you'd need either way when there are complex nested objects inside the model, such as room.

bwindels · 2021-04-29T14:36:37Z

src/components/views/rooms/RoomTile.tsx

+export default class RoomTile extends ModelView<RoomTileViewModel, IState> {
+    private roomTileRef = createRef<HTMLDivElement>();
+
+    constructor(props: IProps<RoomTileViewModel>) {


is there a way to declare a Props type inside the ModelView class already taking the passed in view model type into account?
E.g. something like:

type Props = IProps<V>

in ModelView and then here just do RoomTile.Props instead of IProps<RoomTileViewModel> or something?
This probably doesn't make any sense, haven't done typescript in a while...

Just asking because I would have to export IProps if I were to extract ModelView to its own file.

~~I'm not sure I'm grokking exactly what you're asking but I think you're asking about ComponentProps<typeof XYZ> (React.ComponentProps)~~

Nope don't think this is what you are after, you could use Parameters<ModelView["constructor"]>[0] as the type or something like that to save having to import it again, the above you could wrap in a nicer to read generic type

Interesting, could you give an example of how this could be wrapped?

actually I think I messed that up or have since lost it, /me looks at @turt2live for inspiration

most IDEs will infer the types for people if you just leave it off the constructor, so:

constructor(props) { super(props); // it now knows that props is IProps<T> }

also, imo, given we don't use props but instead this.props in constructors, I'm not too worried about types - we're just fulfilling the contract, which doesn't need explicit type declarations.

The other option would be to make the props the view model, so people can <Component {...model} />

jryans

Looks great to me overall, thanks for writing up the overview, it's very helpful. 😄

docs/mvvm.md

Co-authored-by: J. Ryan Stinnett <[email protected]>

bwindels added 7 commits April 13, 2021 14:14

tag is is not used here

219c35b

extract view model from RoomTile

fcc737d

use vm from parent view

f635164

need to initiate state

c959ce2

use an event for signalling scrollIntoView

b01b8e2

although while testing, the view_room never seems to get dispatched? so it looks like this might be dead code anyway, still porting it though

forgot to update the volume prop in the view model

4c69e53

deal with the view model being swapped out

e4f1653

bwindels added 3 commits April 14, 2021 12:39

remove obsolete tag attribute

d7c7a1c

remove unused code

bf658b5

argh, more unused code

998e4d8

turt2live self-requested a review April 14, 2021 15:17

germain-gg reviewed Apr 15, 2021

View reviewed changes

jryans self-requested a review April 16, 2021 08:39

turt2live reviewed Apr 16, 2021

View reviewed changes

jryans reviewed Apr 22, 2021

View reviewed changes

bwindels added 5 commits April 29, 2021 14:45

Merge branch 'develop' into bwindels/roomtile-vm

0cc503d

extract model binding to ModelView class

2aa5701

use shorthand this.model prop

28ebbfb

explain actions

b9a0af2

fix typing

3f99690

bwindels commented Apr 29, 2021

View reviewed changes

bwindels added 4 commits April 29, 2021 18:47

start writing a doc describing the mvvm approach

f89a976

list properties on view model, so they can be copied to state in view

5667eab

copy view model properties over to component state to avoid un/ rerender

7497587

move code around

6388206

jryans reviewed May 4, 2021

View reviewed changes

docs/mvvm.md Outdated Show resolved Hide resolved

Update docs/mvvm.md

942cc37

Co-authored-by: J. Ryan Stinnett <[email protected]>

MadLittleMods added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Jun 2, 2022

langleyd closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract RoomTileViewModel from RoomTile #5857

Extract RoomTileViewModel from RoomTile #5857

bwindels commented Apr 13, 2021 •

edited by github-actions bot

Loading

bwindels commented Apr 13, 2021 •

edited

Loading

bwindels commented Apr 13, 2021

germain-gg Apr 14, 2021

bwindels Apr 29, 2021

germain-gg Apr 15, 2021

germain-gg Apr 15, 2021

jryans Apr 22, 2021

bwindels Apr 29, 2021 •

edited

Loading

bwindels Apr 29, 2021

bwindels Apr 30, 2021

germain-gg commented Apr 16, 2021

turt2live left a comment

turt2live Apr 16, 2021

bwindels Apr 29, 2021

turt2live Apr 16, 2021

bwindels Apr 29, 2021

jryans left a comment

jryans Apr 22, 2021

bwindels Apr 29, 2021 •

edited

Loading

t3chguy Apr 29, 2021 •

edited

Loading

bwindels Apr 29, 2021

t3chguy Apr 29, 2021

turt2live Apr 29, 2021

turt2live Apr 29, 2021

jryans left a comment

		this.props.model.off("change", this.rerender);
		this.props.model.off("ensureVisible", this.scrollIntoView);

Extract RoomTileViewModel from RoomTile #5857

Extract RoomTileViewModel from RoomTile #5857

Conversation

bwindels commented Apr 13, 2021 • edited by github-actions bot Loading

bwindels commented Apr 13, 2021 • edited Loading

bwindels commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwindels Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

germain-gg commented Apr 16, 2021

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwindels Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

t3chguy Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jryans left a comment

Choose a reason for hiding this comment

bwindels commented Apr 13, 2021 •

edited by github-actions bot

Loading

bwindels commented Apr 13, 2021 •

edited

Loading

bwindels Apr 29, 2021 •

edited

Loading

bwindels Apr 29, 2021 •

edited

Loading

t3chguy Apr 29, 2021 •

edited

Loading