Architecture and design

[kegsay]

Spoke with @dbkr today about the split of React SDK and skins on top. This issue is a huge summary of the discussion and some proposals moving forwards.

Problems with the existing architecture

There's a number of issues currently which I feel aren't so great, including:

• The view/controller approach was intended to allow people to replace the entire HTML(view) but keep the underlying controller logic. In practice, there's tighter coupling here than you'd think and it's not always clear what should go where (e.g. I click a hide/show button which changes state, this is logic so you'd think controller. but it's purely aesthetic information from the view).
• Augmenting controllers doesn't really work. One of the main problems we both identified was Vector's RoomView controller which is a c/p of the React SDK one with some conference specific guff added. This trips people up (both of us included) when instinctively modifying the React SDK's controller and expecting it to work. It doesn't because the Vector one is entirely clobbering the React SDK one, not augmenting it.
• Coupling between controllers and views makes it very easy to introduce subtle breakages. A good example of this is the CreateRoom controller in the React SDK which calls a function which is only defined in the Vector skin (which means it'll crash on anything not vector). A similar subtlety is when views call functions seemingly not defined anywhere (because they're defined in their controller).
• The API surface exposed by React SDK is not well-formed. You can't really make an API reference for it. We have some classes which do some things but there's no real place to get a reference of what you can do aside from examples.

Proposed solution

Rather than splitting along the View/Controller, I propose splitting along component boundaries. In practice, this means:

• Combining the views/controller for a class together.
• Exposing a set of reusable components that the layers on top (Vector) can use.

The biggest concerns with this was "how do you prevent the layers on top from re-implementing all the logic that the SDK does?". My proposal is to make the components extremely thin, stateless UI code and that's it. Currently, the controllers do a lot of "business logic" like making API calls, dispatching actions to the rest of the app, etc. This is good but shouldn't really reside in the controller for a specific component. Take for example resending messages. Previously, you could hit a message and hit Resend which would do the API call and dispatch actions to the rest of the app. When I came to implement "Resend all" in a different component I couldn't easily reuse the existing code to resend a single message because it was tightly coupled with the component it resided in (setting things like this.state and this.props which my new component didn't want).

The proposed architecture would resolve these issues by effectively having "stateless UI components" and "wire components" which glue up other components and make calls through to the React SDK. Every component would accept props which govern their inputs and callbacks that they'd invoke. They would look something like this (using an example of leaving PR comments):

Syntax:
ComponentName(Type propInput, Type propInput)


PullRequestPage(String repo, Integer pr)
     |
     |_____________ PullRequestSummary(PullRequest pr)
     |_____________ CommentListView(Comment[] comments, Boolean showCommentBox, Callback onSubmitText)
                            |
                            |________CommentView(Comment comment)
                            |________CommentView(Comment comment)
                            |________CommentView(Comment comment)
                            |________ ...
                            |________CommentBox(Callback onSubmitText)

An implementation of CommentView would be extremely thin, basically just the render() method:

module.exports = React.createClass({displayName: 'CommentView',
    propTypes: {
        comment: React.PropTypes.any.isRequired
    },
    render: function() {
        return (
            <div className="CommentView">
                <img className="CommentView_avatar" src={
                    comment.getUser().getAvatarUrl()
                } />
                <div className="CommentView_header">
                    {comment.getUser().getName()} commented {
                        comment.getTimeAgo()
                    } ago <a href={comment.getLink()} target="_blank">(Source)</a>
                </div>
                <div className="CommentView_body">
                    {comment.getBody()}
                </div>
            </div>
        );
    }

If you wanted to add any UI-only state (show/hide, etc) it would reside in this file. If you wanted to do anything like say delete the comment, you would add a new prop onDeleteClick which will be called when they hit the delete button. This allows massive code reuse. For example, in Github's case you hit two different endpoints depending on whether you're interacting with Line Comments or PR comments. This UI component need not care, it just knows how to display the comment and a delete button. If one of the APIs didn't expose a deletion endpoint, you could easily add a prop to hide the delete button (or do inference e.g. "if there is a delete callback, I'll show a delete button").

By using props everywhere, we encourage "bubbling" of callbacks up from children to parents (See how onSubmitText is bubbled up from the CommentBox to the CommentListView to the PullRequestPage). This is a bit boilerplatey but is situationally useful (e.g. to disable the parent UI if you're sending a message from the child). There is no hard and fast rule on whether you should bubble or not other than asking if the component you're writing might be reused in another part which may have different logic (think the GH commenting example). Both uses are valid.

Dispatcher

The dispatcher would remain similar to as it is today but with a much stricter split between React SDK triggered actions and other stuff (e.g. Vector-specific triggered actions). Layers on top of the SDK would be almost exclusively consumers of SDK actions from the dispatcher, and would be able to produce/consume actions in their own layer with a sensible prefix e.g. vector_. Layers on top may in rare cases want to emit SDK-level actions for more exotic behaviour but it should be rare. Doing this means if you're following through some code you can have greater intuition of the control flow between the different layers based off a helpful action name.

Business logic

This relates to the "resend" example I gave earlier. The proposal here is to have a set of "pure logic classes" which:

• Make JS SDK API calls.
• Emit actions on the dispatcher.
• Manage any internal state (e.g. call state).

We already have started to organically grow these with CasLogic, CallHandler, SlashCommands, TextForEvent, WhoIsTyping and so on. I propose we formalise this concept (e.g. into a logic directory) split on concept boundaries (Calls, Messages, Auth, /commands, etc). These can either be "statically" linked into the React class (like we currently do by require()ing them) or by DIing them as additional props. This brings me onto...

Testing

Testing React components and UIs in general is Hard. We basically have two options. Test visually by comparing to "known working" snapshots and test by using shallow rendering (basically it dumps render()'s HTML output to a string which you can then test for equality. I dislike both of these. A change to the HTML <div> ordering shouldn't break the tests IMO. To this end, I do not propose testing React components directly, but instead the business logic classes. This is one of the reasons why the proposed architecture has extremely thin UI components with not a lot that can go wrong in them.

Example control flow

• [MessageComposer] User hits a button to send a message. Invoke a prop callback with the raw input string.
• [BottomRoomBar] Maybe augment the raw input string (e.g. imagine if a "Markdown" check box was enabled) then invoke a prop callback.
• [RoomView] Call through to a logic class e.g. MessageController.
• [MessageController] Do the API call and get a Promise. Maybe immediately emit a "sending" action on the dispatcher so other UI components not on the critical path can grey out their buttons. Attach a resolve/reject listener to emit a "sent" or "failed to send" action. Return the Promise.
• [RoomView] Return the promise.
• [BottomRoomBar] setState to display a spinner. Attach a resolve/reject listener on the promise to hide the spinner depending on the outcome. Return the promise.
• [MessageComposer] Grey out the UI. Attach a resolve/reject listener to un-grey the UI after it succeeds/fails.

This gives components a lot of flexibility. If components are on the critical path of parent-child relationships, more often than not they want to do stuff so rather than forcing them to listen for the "dispatcher" actions (which emulate a request/response cycle which is by far better represented as you know, a function call) they can just hook in before the bubble back the response. Furthermore, in this control flow everything is explicit. Your boundaries (the props) are well-defined.

Outstanding problems

• Augmenting components (the RoomView example) remains a thorny issue. This modified the render() method and added in extra props to the React SDK RoomView. With RoomView as it is, there is no good solution to this. You can't even abuse mixins because you can only have One True render() method which would need to know the "base" React SDK HTML and then the "extra" Vector HTML on top. To this end, a rather unsatisfying (though probably correct) solution would be to keep as much shared code as possible and then clobber the rest. In practice, this would mean breaking RoomView up into smaller stateless UI components and then switching to the Vector-specific code higher up the chain when you're just gluing together components. This results in less code duplication and prevents you from opening the "wrong" file (because you're loading a different component entirely rather than the same component with a different controller backend.

---

Next steps

If @ara4n and @dbkr are in agreement, the "actionable" steps to take on this would be:

• Go through on a component-by-component basis and combine the view/controller. Suck out anything which is not exclusively UI-related into a logic class. Aggressively define and inject props when you need some data to work with (rather than yanking it from MatrixClientPeg) and think about what callbacks you want to provide (e.g. RoomList may want to just have onRoomClick(Room), MessageComposer may want onTabClick(inputText), onSubmit(text), etc). Make Vector either use the pure-UI class that is left if the HTML is right, or write the pure-UI class and use that instead. This can be done incrementally for each component without requiring one huge branch/PR/merge.
• If Vector emits any actions specific to it (e.g. conf call stuff?) then add the vector_ prefix.
• Start adding tests for the logic classes we've pulled out of the components.

What the React SDK API reference would look like

• A list of Components each with:
  • A name.
  • A list of defined properties with types (from propTypes) documenting the inputs/callbacks.
  • A list of actions that it will be listening for.
  • A list of actions that it emits (with args).
• A list of "logic" classes with JSDoc. Any component is free to directly call these.
• An "action" reference. This is a complete list of actions that the SDK emits each with:
  • A name.
  • Names of keys / values with types on the payload.

Most components will be stateless UI components but some components may be "wire" components which could form the basis of Matrix React Console (though it would need CSS).

[kegsay]

At request, a wire component (going with the example from before):

var LogicClass = require("../some/logic/class");

module.exports = React.createClass({displayName: 'PullRequestPage',
    propTypes: {
        repo: React.PropTypes.string.isRequired,
        pr: React.PropTypes.integer.isRequired
    },

    componentWillMount: function() {
        this.setState({
            pullRequest: LogicClass.downloadPullRequest(this.props.repo, this.props.pr)
        });
    },

    onSubmitComment: function(text) {
        return LogicClass.sendComment(this.state.pullRequest, text); // returns a Promise
    },

    render: function() {
        return (
            <div>
                <PullRequestSummary pr={this.state.pullRequest} />
                <CommentListView
                    comments={this.state.pullRequest.getComments()}
                    showCommentBox={true}
                    onSubmitText={this.onSubmitComment} />
            </div>
        );
    }
});

[ara4n]

TL;DR: "pass data bidirectionally between components via props, don't split views & controllers (using aggregation rather than inheritance for extensibility) and be aggressive with stateless view components as the main UI building blocks".

[dbkr]

MatrixChat is also a very good example of where we need a better split of app business logic from UI control logic: there's a lot of code in this controller for startup / teardown of the chat app and I think its purpose should be confined to just making the UI do what it's supposed to. This we should definitely do and was the goal from the start, but often fell by the wayside because of time constraints and the fact that because the js-sdk handles so much that the app/react-sdk's business logic is (was?) quite thin anyway.

The part of this I'm most hesitant about is the wire/stateless components. In the tree of MatrixChat- > RoomView -> RoomList -> RoomTile, which of these do you think would be which type of component? I think the implication is that stateless components are also logic-less components? If so, this to me implies that a lot of logic (or at least the code that calls into the logic classes) migrates upwards away from the leaf components and towards one place, reducing modularity quite a lot. I don't fully understand what the proposal is here.

With regards to removing the split between controller and view, we should probably look at how do-able it would be to do away with replaceable HTML and how easily we could have vector and react-skin using this method of replacing entire components if we need to change the HTML: I'm concerned this might end up making things worse if we end up having two copies of lots of UI components to maintain. The current system was designed as it was because being able to substitute the HTML was a specific requirement.

[dbkr]

Also, I'm assuming the factoring out of business logic to separate logic classes doesn't apply where the business logic is a single line call to the js-sdk or similar?

[kegsay]

I think the implication is that stateless components are also logic-less components?

Yup, that is indeed the implication.

If so, this to me implies that a lot of logic (or at least the code that calls into the logic classes) migrates upwards away from the leaf components and towards one place

Yup, that is the intention.

reducing modularity quite a lot. I don't fully understand what the proposal is here.

This part I think we're crossing wires at. This increases modularity of the UI components (e.g. allowing me to reuse my CommentBox for PR comments and Line comments). This decreases modularity of "wire" components (because it now wires UI and logic glue rather than just UI), but that is precisely the point. These "wire" components are the core essence of the app (the layout of the page and what happens when stuff on the page is clicked), so the intention is that these would be changed to fit. This duplication though is fine; there is minimal maintenance burden because these components are literally just tying things together, there's basically nothing to them. Common UI code can be reused and common logic code can be reused, both of which are least likely to want to be changed (though both possible to change if you're that way inclined).

being able to substitute the HTML was a specific requirement.

The stateless UI components are basically just HTML (check the CommentView example I gave, the class is basically just the render() method!

Also, I'm assuming the factoring out of business logic to separate logic classes doesn't apply where the business logic is a single line call to the js-sdk or similar?

Depends on your willpower 😄 it's very tempting to allow one-liners to become two, then three and more. Proxying through for a one line call initially is a bit annoying but protects against organic growth of logic as we realise "oh wait, I also want to do this after that". People unfamiliar with the project will be unlikely to think "okay this is 2+ lines now, better make it a logic class".

[ara4n]

Much discussion IRL to try to crack the remaining questions over structuring react-sdk customisable without introducing maintenance problems. Conclusions were:

• Let's go ahead and migrate the architecture over from Views + Controller-mixins to being lightweight View components + wiring View components + Generic business logic classes as Kegan suggests.
• matrix-react-sdk becomes a repository of all reusable components - all the way up to the application layer. It's built to be as generic as configurable possible, so hopefully folks using components will not need to fork them, but instead can pass config into the components to customise them. (e.g. a UserSettingsStore logic class could support managing generic settings, rather than being limited to some arbitrary subset).
• matrix-react-sdk contains basic CSS - plain white cosmetics; no frills or slow-downs, but usable.
• matrix-react-skin dies as an unnecessary abstraction.
• matrix-react-console is then a wafer-thin layer on top of matrix-react-sdk's MatrixChat react component - just a single index.html pulling it in with the default matrix-react-sdk CSS.
• vector is then a more customised layer on top of matrix-react-sdk; adding additional CSS, views and logic classes for whatever frills and branding it adds.
• vector (and similar customisations) use Dave's existing sdk.getComponent() stuff to allow for custom components to replace the default ones supplied by matrix-react-sdk, just as they are today (but without any of the controller mixin stuff).
• N.B. whilst a customised app could extend both react components and logic classes via mixins/inheritance, i'm assuming we strongly recommend that they replace the component outright or add their own custom logic class in the app-specific layer. E.g. if Vector added a new set of Settings management functionality, this could be VectorSettingsStore and a VectorSettings component in the application layer, rather than trying to extend UserSettings. Whilst this might encourage some forking of view components (e.g. the developer might choose to replace the UserSettings view component entirely with VectorSettings by forking it, and then have maintainability problems between react-sdk and vector), we can hopefully discourage this by making components sufficiently granular that swapping one out doesn't duplicate much code... or by making the react-sdk components sufficiently customisable and generic that one can just reconfigure them (with decorator-style code patterns) rather than forking them.

The only big pending question I can see is whether we should do better at compartmentalising the CSS. Specifically: rather than having to rely on a CSS reset+override to replace the react-sdk's CSS, should we have a mechanism similar to sdk.getComponent() which lets us replace the CSS entirely for a given component? E.g. if I've specified a custom UserSettings component, I would want the option of either totally replacing or extending the base CSS provided by matrix-react-sdk.

[dbkr]

Sounds good. There is very little structure in place currently for managing CSS. We need some, especially since we now need CSS from dependent packages (gemini scrollbar) which is currently done is a way which breaks on different versions of npm. Perhaps we should make this able to replace the CSS for a particular component.

[kegsay]

i'm assuming we strongly recommend that they replace the component outright or add their own custom logic class in the app-specific layer.

Absolutely. We can by all means make our components flexible (a decent amount of props to configure the look and feel, CSS, etc) and our logic classes flexible (generic user settings vs specific-to-vector settings) to make it such that for 90% of use cases they are appropriate. There is also the decorating approach you touched on so they just wrap the component with that little extra something they require.

Perhaps we should make this able to replace the CSS for a particular component.

+1. I would be interested in knowing what value (if any) LessCSS gives us.

[kegsay]

This has now been done.