Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sugar api for state/props configuration boilerplate #136

Closed
mairatma opened this issue Jul 26, 2016 · 16 comments
Closed

Sugar api for state/props configuration boilerplate #136

mairatma opened this issue Jul 26, 2016 · 16 comments
Assignees

Comments

@mairatma
Copy link
Contributor

mairatma commented Jul 26, 2016

One feature that we've promised to work on is to create some kind of sugar api for configuring state/props, so that common use cases can avoid boilerplate.

The plan is to add that to version 2.0.0, which has already been released, but I wanted to first get more details about how everyone would prefer this to work.

From what we talked about the most common use cases are of using just the validator. That requires you to type something like this:

MyComp.STATE = {
  foo: {
    validator: someValidatorFunction
  }
};

So we could allow a simpler usage, where if you set the config to a function we'll assume that you're passing just a validator.

MyComp.STATE = {
  foo: someValidatorFunction
};

If I remember correctly we also talked about passing the value directly as the config, if only value was to be set, like this:

MyComp.STATE = {
  foo: 2
};

That would conflict with the validator sugar though, since value can also be a function, so in that case we won't know which of the two it's supposed to be. So, if validator is the most useful one we could add just that.

I was thinking about this though, and thought that instead of adding this sugar directly on Metal.js, we could instead create a new project (or maybe change metal-state-validators for this) to build and return the right config object that Metal.js expects. That way we can have an api that could work as sugar for more use cases.

For example, instead of something like this:

MyComp.STATE = {
  foo: {
    required: true,
    validator: validators.number,
    value: 13
  }
};

We could have the option of doing something like:

MyComp.STATE = {
  foo: Config.number().value(13).required()
};

Each of these calls could return an object with a config key that has the format Metal.js expects, as well as more functions you can call to add data to it. In Metal.js we'd just need to accept an object with a config key that has the expected format to have this work.

What do you think?

@bryceosterhaus
Copy link
Member

One thing to note is that when dealing with STATE vs PROPS. We almost never pass a validator into STATE because it is internal and a specific type should already be expected.

Currently in our project we have been doing something like this where createState is our own helper we have created.

const CONFIG = {
    onChange: Types.func, // Default is to accept validator function
    editing: Types.bool,
    stickyTime: Types.number
};

const STATE = {
    items_: []   // Default is to accept initial value
};

Component.STATE = createState({CONFIG, STATE});

Now the one thing we could add on top of this is what you mentioned Config.number().value(13).required() although this is something that would likely only be used for props.

So if this was included within metal's api plus a metal-state-validators change a basic example might look like

Component.PROPS = {
     active: Types.bool.value(false),
     id: Types.number.required(),
     onChange: Types.func
};

Component.STATE = {
     items: [],
     open: false
};

I like the example above, but it may be strange that we handle STATE and PROPS differently with the initial values.

@mairatma
Copy link
Contributor Author

Yeah, I think it's best to keep both using the same api. When using soy we don't separate between state and props btw, there's only STATE and you can use a property to indicate if it's going to be internal or not. Also, validators can still be useful even for internal state, developers can still pass a wrong value by mistake, or get it from somewhere else, and this can help debug it. It'll be more consistent to have it work the same way everywhere, and also more powerful.

Besides, the STATE property's value could be an object, so how do you currently distinguish that case? Do you always assume it's going to be given just a value, or don't allow object values with this syntax?

Also, I think that Types, or validators (as we call metal-state-validators today) wouldn't be a good name, since we'd not only have types, but also required, value, as well as potentially any other config option that can be used (like setter for example). Something like Config, Data or DataConfig would probably be best.

@bryceosterhaus
Copy link
Member

One thing we will have to worry about is shapeOf and oneOfType validators. Those might get messy if we go the route of chaining .required and .value.

Ill have to think about this more and look at our use cases

@mairatma
Copy link
Contributor Author

Could you explain why they'd get messy?

@bryceosterhaus
Copy link
Member

bryceosterhaus commented Jul 26, 2016

Say we have something like

STATE = {
    foo: Config.shapeOf(
        {
            bar: Config.string,
            baz: Config.number
        }
    ).value(
        {
            bar: 'test',
            baz: 3
        }
    ).required
}

Just seems messy in my opinion. Granted, this is also just how Liferay would require formatting standards. But then again, if we have a shape and default value of an object, there may be no way of getting around that. So you may be right actually. It might not be as bad as I think.

@mthadley
Copy link

mthadley commented Jul 26, 2016

One more thing, which may belong more over at metal-redux, but I think is related to this discussion, is how we describe props received from the store.

To the component being "connected", all values received from the store and mapStateToProps are just props. However we found that describing them and their types inside of PROPS was hard to read, since most of the time you only compared about props that you needed to actually pass when using the component in your app.

Were wondering if this is something that should be achieved through an API, or maybe just source formatting.

@mairatma
Copy link
Contributor Author

mairatma commented Jul 26, 2016

@bryceosterhaus since this is supposed to be sugar it means that you don't have to use this syntax every time. In the use case you showed it'd maybe be better to just use the original format instead, since it's big anyway.

@mthadley: yes, I think this would live in metal-redux like you said, since Metal.js doesn't know about it at all. We could add the option of setting the props used in mapStateToProps in a different place like you suggested. Could you open an issue for this there with the proposal? It shouldn't be hard to do, I was just focusing on the other bigger changes first.

@mthadley
Copy link

Created a related issue at metal/metal-redux#8.

@bryceosterhaus
Copy link
Member

So after looking into it a bit more, I like your proposal.

Would it be possible to import the Config utility via metal-jsx?

Here is a sample of how we would be using it in our components with also having a store

import Component, {Config} from 'metal-jsx';

class MyComponent extends Component {
    //...
}

MyComponent.STATE = {
    count: Config.value(5),
    loading: Config.value(true)
};

const STORE = {
    creator: Config.instanceOf(Map).required
};

MyComponent.PROPS = {
    ...STORE,
    id: Config.number.required
    items: Config.array.value([])
    onChange: Config.func.required
};

Config would be able to utilize metal-state-validators specifically for types.

@mairatma
Copy link
Contributor Author

Config is something that can also be used on non jsx projects, so it'd be better if its code isn't inside metal-jsx. That said, we could have metal-jsx import and export it, so that you don't need to import another module, since that will be heavily used in jsx projects (as it resembles react's api for types). Would that be ok?

Another thing is that in the way I'm thinking of this you'd actually need to call each config function, not just reference it, since each call will need to make some changes to the returning object for Metal.js to use it later. So the api should be more like Config.number().required(), not Config.number.required. That will also make usage more consistent, instead of having only some options requiring calls while others don't. I realize that React manages to do this without calls, but that's because their use case is simpler, they only have types and isRequired, so there's at most one chain call, while we will have more options to chain (like value, and possibly others like setter later too).

@bryceosterhaus
Copy link
Member

That would be great. Both cases work for me.

@ethib137
Copy link

@mairatma this sounds great! Do you have an idea of when you'll be able to work on this, or if you would like us to help in any way?

As a team we've realized that before we are able to release Loop using Metal we are going to need to convert to Metal 2.x due to the way it handles refs. We also don't want to spend the time converting everything to the new way of handling State and Props until we have this sugar syntax ready as well.

Thanks.

@mairatma
Copy link
Contributor Author

Sure, since we're agreed I'll see if I can do this today, and have it ready tomorrow at the latest so that you can start converting :D

@eduardolundgren
Copy link
Contributor

This Config sugar api is a good improvement without compromising old codebase.

@mairatma
Copy link
Contributor Author

Done, this is already available in version 2.1.0 :)

@mairatma
Copy link
Contributor Author

Oh, and just to make it clearer, we've moved metal-state-validators to this repo, since it's also required for the sugar api to work, and it's really quite useful. I've added an explanation to the original repo's README file about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants