Architecture and design about matrix-react-sdk HOT 9 CLOSED

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>
        );
    }
});

from matrix-react-sdk.

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".

from matrix-react-sdk.

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.

from matrix-react-sdk.

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?

from matrix-react-sdk.

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".

from matrix-react-sdk.

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.

from matrix-react-sdk.

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.

from matrix-react-sdk.

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.

from matrix-react-sdk.

This has now been done.

from matrix-react-sdk.