PropTypes and Prop Validation with JSX

[ethib137]

One we have found very helpful while working with react is the way that they have implemented PropTypes: https://facebook.github.io/react/docs/reusable-components.html We have found that it helps to self document code and point out issues sooner by logging errors when propTypes do not match up correctly.

Would it be possible to implement this type of feature in Metal JSX?

[yuchi]

It is already implemented as attribute validators, check them out.

[mthadley]

Taking a look at the docs here, I think we would do something like this:

class MyLabel extends Component {
  render() {
    return <span class="label">{this.config.label}</span>
  }
}

MyLabel.STATE = {
  label: {
    validator: core.isString
  }
}

It also seems like this.config and STATE can be considered a combination of props and state in react. Since they would both be described in STATE, I'm curious if there is a good way to show that one attribute is expected to be passed in, and another is expected to be private and managed by the component itself.

In YUI, there was a readOnly property for attributes, and then the component could call this._set internally.

[mthadley]

Another subtle difference with PropTypes and STATE, PropTypes are only checked in development mode and throw a warning. This means that they do not incur a performance penalty for the end user.

Edit: To expand on this, PropTypes do not affect runtime state. If a prop fails a check, it only prints a warning and nothing else. The component will happily continue on its way with the "invalid" prop. This is much different than a STATE validator, which will actually ignore the value. So in this way, they have fairly different purposes and effects.

[yuchi]

the nice thing of what Metal inherited from YUI is the no-privates approach. You have a state, someone owns you, they owns your state too.

It can lead to more fragile practices but it's very flexible in the medium run.

In YUI you had initOnly (or something like that) that made it clear you needed the attribute as part of the initial config object. Don't know if Metal has it too.

[mthadley]

With some quick experimentation, we noticed some interesting behavior:

In the example above, I expected the validator to be called with val being 'foo', and that the value property of the STATE attribute would be more like a default value. Instead the validator is not called, the only thing that recieved the 'foo' value was this.config.

I suppose we could then do something like this:

Edit: Ignore this comment, it seems to actually be working as expected. May have had a typo or something.

[mairatma]

Thanks a lot for explaining this so well @yuchi! :D

Just confirming, as @yuchi said, we indeed do not have a separation between internal only data like React does. We preferred leaving that to the developer's own organization. You can have internal state if you wish, and signify that with a leading or trailing _ for example, which is how it's usually done for regular properties and methods. That's what we've been doing, and it's worked well so far.

As for the validators, @mthadley, the default behavior for them is to just ignore invalid values. But our STATE configurations allow you to customize this behavior if you wish to do so. For example, you can log warnings yourself and return true from the validator. That said, if you believe this behavior is something you miss, we could work on adding some feature where you can return something else in the validator to have it warn instead of ignore. Or, maybe even better, we could have a utility class similar to PropTypes providing validators you can use that would follow that pattern. What do you think?

[mthadley]

@mairatma Thanks! I like that idea, that would be a very nice utility to have. I think anyone else coming from React would also be happy to have that available to them. We could also probably find a way to strip out those checks in production code, whether its through the build system or some other method.

Also, thanks @yuchi for your replies. It was very helpful!

[cirocosta]

Have you taken a look at how react does @mthadley ?
Remembered me of the old way of doing in C:

#ifndef NDEBUG

#define DASSERT(condition, message) ASSERT(condition, message)
#define DLOG(message) LOG(message)
#define DLOGERR(message) LOGERR(message)

#else
#define DASSERT(condition, message)
#define DLOG(message)
#define DLOGERR(message)

#endif // ! NDEBUG

i.e, macro all the way hahah

---

also, maybe we shouldn't get too biased about how react does things (not saying that they're doing it wrong)

[mthadley]

@cirocosta I know there is a webpack loader that does something like that: https://github.com/friskfly/if-loader

Good point. We should remember that Metal is not React. We can still make use of good practices and ideas we learned elsewhere, but we should also consider what is the Metal way of doing it.

[mairatma]

@cirocosta, @mthadley +1! I'm glad you guys understand that :)

That said, I know that there certainly are good patterns and ideas in React that we can learn from and use to improve our library, and those are worth thinking about. And in some cases, like in this validator one, some developers would prefer if the behavior was different. So having this separate utilities library that you can use for your validators is ideal here, since we don't need to change the default behavior but can also provide a different one.

Unfortunately I'm very busy these days, so I don't think I'll be able to get to this very soon. But let me know if any of you could help out with this idea. We could create a new repo for this and you could send me prs. Might be a good way to get more used to Metal.js (at least the core library, which has some type checks that could be used here, as well as the pattern we're using for our repos) :)

[bryceosterhaus]

@mairatma, I can start working on it today. If you want to create a repo, just send me the link and I will send you any prs

[mairatma]

Thanks a lot, @bryceosterhaus!

Any idea what name we should use for this? I don't think there should be the word prop in it, since we don't have props. I've created the repo as metal-state-validators, but if you guys have a better name we can change it.

[mthadley]

I imagine that is probably fine for most people.

If we want some other suggestions, here are a few that come to mind:
metal-type
metal-types
metal-state-type
metal-state-types

[mairatma]

I think I like metal-state-types best, it's smaller than metal-state-validators, but not too generic

[mairatma]

@eduardolundgren pointed something out to me now which makes sense. These utilities will be validator functions in Metal.js, so the name types may not fit well, while validators would. I think we have to see how the api will be before defining the name, and see which will fit better in Metal.js's case. So we can keep metal-state-validators for now and maybe change it once it's done.

[eduardolundgren]

The name validator gives us more space for having other kinds of validators not only type checkers. And seems like PropTypes from react realized that, and they have dozens of keys on its contract in order to cover different edge cases not type related.

[bryceosterhaus]

Makes sense to me.

One thing I did notice with the validator, is that it only passes one argument, value. Would it make sense to also pass in the context of that component?

Something more along the lines of

file: metal-state/blob/master/src/State.js

    callValidator_(name, value) {
        var info = this.stateInfo_[name];
        var config = info.config;

        var renderer = this.getRenderer();
        var component = this.constructor.name;

        if (config.validator) {
            return this.callFunction_(config.validator, [value, name, component, renderer]);
        }
        return true;
    }

This helps for our specific use case so that we can alert the developer as to which state attribute and which component this is happening in. Also passing in the renderer can give us context of the parent component if it is nested.

Not sure if there is an easier way to do that since we may not want all of those args for non type-checking validators.

[mairatma]

When we call validators we always do it in the component's context, as you can see here: https://github.com/metal/metal-state/blob/master/src/State.js#L192.

Unless the validator function is previously bound by another context you should be able to get the component instance via this inside it, and that way get access to both its name and renderer if you wish.

[bryceosterhaus]

@mairatma, that works for component and the renderer. However, the key of that value is prone for error.

We can try to derive the key from this.config, but there is a chance that multiple keys have the same value.

Would it be okay to call the validator with value and name?

return this.callFunction_(config.validator, [value, name]);

This way we at least insure that the value the validator operating on will always match its key.

[mairatma]

Sure, makes total sense. Could you send a pr with this change to metal-state? Besides this change it'd just require adding a test case for validators to check that the name is being passed as well :)

[mthadley]

This change is also inline with YUI Attribute which I know inspired parts of metal-state. See validator: http://yuilibrary.com/yui/docs/attribute/

[bryceosterhaus]

Yeah I will go ahead and do that now. Thanks @mairatma!

[mairatma]

Now that metal-state-validators already exists we can track further requests related to it there, so closing this issue :)