Writing Maintainable Components, Best Practices.

[mthadley]

I'm wondering if we should define a set of guidelines for writing maintainable components. Metal gives the developer a lot of freedom when it comes to how they can organize their components, but this comes at a cost.

I know it has been brought up before, but React has some simple ways that it encourages you to write reusable components. Things like the separation of prop, state, and propTypes (and now flow) are examples of this. propTypes was the inspiration for metal-state-validators, which is useful because it both documents your component, and also gives you useful warnings.

Of course you don't have to write code using the "one-way data flow" or React mindset, and I'd like to hear other's thoughts on how to architecture large applications in Metal.

As an example, here is an example (somewhat poor on purpose) of how things can become messy:

import Component from 'metal-jsx';
import Types from 'metal-state-validators';
import {connect} from 'metal-redux';

// ...

class User extends Component { /* ... */ }

User.STATE = {
  id: {
    validator: Types.number,
  },

  name: {
    validator: Types.string,
  },

  age: {
    validator: Types.number,
  },

  count: {
    validator: Types.number,
  },

  like: {
    validator: Types.func
  },

  friends: {
    validator: Types.array,
  },

  ping: {
    validator: Types.func
  },

  enemies: {
    validator: Types.array,
  }
};

export default connect(
  (store, ownConfig) => {
    /* ... */

    return {
      friends,
      age
    }
  },
  {
    like,
    ping
  }
);

If I wanted to use this User component, how do I know what I need to pass to it? It's not enough to just look at the STATE config. I need to cross reference it with my mapStateToConfig, mapDispatchToConfig, and also determine which of the STATE attributes are managed internally by the component. Realistically, I could probably do a better job here, like prefixing the private state variables with an underscore, or maybe naming my action creators differently (pingAction instead of just ping). However these are best practices that need to be defined, and that's what I am trying to bring up here.

Good naming schemes are nice, but if something is important enough, it may be something that should be easy to enforce through the framework's API.

For example, here is an imaginary Metal-like API for the same component:

// This could be considered private
User.STATE = {
  count: {
    value: 0
  },

  enemies: {
    value: []
  }
};

// Now I know I only need to pass in these:
// <User id={2} name="mike" />
User.CONFIG = {
  id: {
    validator: Types.number,
  },

  name: {
    validator: Types.string,
  }
}

// STORE could be something that metal-redux picks up
User.STORE = {
  age: {
    value: (state, ownConfig) => /* ... */,
  },

  friends: {
    value: (state, ownConfig) => /* ... */,
  },

  like,
  ping
}

export default connect(User);

This isn't a proposal for any API changes, I just wanted to think about how best practices might influence API changes in Metal down the road.

Thanks, and I am very interested to hear what other people or teams are doing when it comes to this!

\cc @bryceosterhaus, @ethib137

[mairatma]

You have a good point.

I think that for indicating which state properties are private vs public using underscore, like you mentioned, should be enough. That's what I've been using so far and at least to me this is enough for that purpose.

Not sure what should be done for properties coming from the store though. I didn't find anywhere showing best practices for this when using react-redux, the examples I see don't try to distinguish regular props from store ones. Do you have an example of code doing something like this so we can have an idea?

From what I understood actually, an advantage of connect was that you could have two versions of a component: one that works completely independent of the store (just by receiving props), and another that receives some of its props from the store. That way you could use the original component in different contexts, without relying on a redux store every time (which helps write unit tests for example).

So having that in mind I think that a separation via a STORE property doesn't feel like it would be a good practice. It would tie the original component even more with the store. Again, a naming convention feels better for me, besides being more simple. But, like I said above, I haven't seen people try to do something different, so maybe that's why I can't picture it.

I'd also want to learn other people's opinions on this, this is a good discussion to have, so let's make use of this opportunity :)

[bryceosterhaus]

Naming convention for private/public methods with an underscore is definitely useful, but I still think there is something to be said about having an api distinction between public and private state.

The big question for metal right now is "what is best practice and implementation?" Component life cycle is very similar to react and fairly easy to pick up, however the flow of data can be confusing as to what is the best approach or the "metal way" of doing things.

Just taking an example from the metal docs, there are multiple ways to respond to data changes both within the component and also from the parent component. Take a look at just a few examples below of handling data-flow in metal

var obj = new Calculator();

obj.on('numberChanged', function(event) {
// event.prevVal has the previous value.
// event.newVal has the new value.
});

class Parent extends Component {
        onChange(newNumber) {
              this.number = newNumber;
        }
    render(newVal) {
        return <Calculator number={this.number} onChange={this.onChange} />;
    }
}

class Calculator extends Component {
    onChange_() {
               this.config.onChange(this.number + 1)
        }
}

class Calculator extends Component {
    syncNumber(newVal) {
        // respond to change
                // Could potentially emit this.config.onChange that is passed from parent
    }
}

So you can see, from a developer who is try to use metal in their app, it can be really confusing as to which approach to choose. And sometimes having so many different options can be more of a headache to navigate.

React was explicit in their "one-way data flow". A component would never alter its props, since those are always provided from where it is consumed. A child component only changes the parent's state by using a callback prop. This was super useful in that we knew exactly how data was supposed to be handled.

My thought for metal is that there needs to be some sort of best practice or guidelines of how data flow is supposed to be handled. It seems like there are 10 different ways to change state for a single component. I'm not saying we need to copy React and provide state and config but if we can figure out some sort of data-flow guidelines it would be a huge benefit to metal and its adoption. As we continue to migrate Loop, it is only getting messier and more confusing as to how we handle that data flow.

[eduardolundgren]

Before you guys started using it we didn't even have config (props) concept because in my experience it wasn't necessary. The flow of data always happened from component to state and that was enough.

After requests, we agreed that having config concept doesn't hurt since it's more like a read-only view for values passed to the constructor of the component. If you guys think it's confusing we can decide what to do with it while it's not in final version.

We also have the events xyzChanged and the helper syncXyz helpers - those two I agree can be trick to know why them exists. The syncXyz helpers are basically a shortcut to not have to bind the event all the time. Maybe we don't even need to document the sync helpers?

Also, is important to pay attention and not over-engineer the flow based on all different theories put on the table by those new frameworks, where in reality the problem is simple if taken correctly.

I think it maybe time for us to do a meeting to explain the parts you described is getting messy. Either this may be an alert for us to dry down our api or we can just point to the best practice in case they fill the gap you are having.

[natecavanaugh]

Just my 2 cents:
I think of config (props), not as a read only config that you only pass to the constructor, but a definition of what can be modified externally by a consumer of the component.
So in YUI terms, it would be the ATTRS object, and those properties can be modified on the component at any time (in YUI it was with .set or .setAttrs).
State would be private (like in React, but I don't think we have to copy their nomenclature 100%), though this leads to questions of how to modify this state.
For instance, is it simply a property that anyone technically could modify, but we tell them not to?
Or do we implement some way of ignoring external state modifications, and only allow it to to be changed internally? (I'm not even sure if this is possible without something like ES6's Proxies).
Do we place all of those on a state object (ala React), or some other object property we maintain on the component?

Personally, I think the uni-directional data-flow is the easiest to go towards for end developers who are building components.
I remember us struggling quite a bit with YUI's event/attribute syncing lifecycle, and we always trying to figure out which part of the lifecycle to do certain modifications, and using a src object to prevent race conditions and infinite loops, like:

var SRC = {src: 'ui'};
A.Component.create({
....
bindUI: function(){
    this.on('fooChange', this._afterFooChange);
}
someMethod: function(){
    this.set('foo', SRC);
}
_afterFooChange: function(e){
    if (e.src !== SRC) {
        // respond to user modification
    }
}
});

So I can definitely see some benefits to allowing a single direction of data flow, but of course, there may be other approaches where you guys can think of that would be better.

[mairatma]

Thanks for all the feedback everyone! This was really helpful.

Me and Eduardo also had a meeting with some of you last Friday, but I forgot to document here the conclusions we came up with then.

We all agree that having a way to distinguish public/internal data is important, as not having it is causing many of the current problems related to best practice confusion as well as some technical problems too. On Friday we've agreed to offer this distinction as well as different namespaces where they can be accessed (state and props). This will be much better to use with JSX, as it's more familiar, easier to read and will allow for things like passing all props down via spread in jsx calls (which today meant passing all properties from the component instance).

Me and Eduardo realized that this namespace separation won't be so good for soy though. That would make passing data down to sub components a lot harder inside soy templates for example, besides other problems. The template you use ends up affecting the way you use the component, and specially the component's data, which is why we've had to change many things recently related to JSX usage which aren't issues with Soy.

So we've decided to build a way to handle component data differently for different templates too. By default we'll keep our current behavior, of having all data be accessed directly via the component instance (though now you'll be able to specify which are internal only). For JSX components data will be automatically split between state and props though. That way we can cover both use cases well.

All this (plus the change in the approach of reusing components, explained here) will require some bigger changes than usual though. It'll be risky to publish it by just incrementing rc again. This version of Metal.js is quite stable though, so we've decided to go ahead and finally release a 1.0.0 version with the current features, so that these changes (which I'm already working on, and will mainly just affect JSX usage) can be released as a 2.0.0.

[mairatma]

This issue was moved to #3

[mairatma]

This issue was moved back.

[mairatma]

The changes we've agreed on here are now available on version 2.0.0.

I'll be updating our other repos that will be affected by these changes (such as metal-redux and metal-react) to use the new version soon.

[bryceosterhaus]

@mairatma, with the new API changes in 2.0.0 were you going to add the sugar-api to reduce the boilerplate or was that something you were expecting projects to do on an individual basis?

I remember talking it about it in one of our meetings, but wasn't sure what the expectation was. We can also open a different issue and talk about there if that is easier.

[mairatma]

Yes, that's also on the roadmap, just hasn't been done yet. I'll open another issue just for this feature so we can track that better :)

[mairatma]

I've created #136 for this. Check it out when you can, I have some proposals there and would like to know which one people like best :)

[bryceosterhaus]

perfect. Thanks!