Config.required() is mutating the object

[carloslancha]

We made a reusable validator:

import {Config} from 'metal-state';

const itemShape = {
	active: Config.bool().value(false),
};

const itemsValidator = Config.arrayOf(
	Config.shapeOf({
		active: Config.bool().value(false),
	})
);

export {itemsValidator};
export default itemsValidator;

But when I import it and use it in a component as a required param:

import itemsValidator from './items_validator';

MyComponent.STATE = {
      items: itemsValidator.required(),
};

The object mutates to always be required, so if I use it again in another component it will be required even if I don't want to...

[robframpton]

Hey @mthadley @bryceosterhaus

Since you guys use these helpers a lot, do you see any potential issues with this approach? I just sent in a PR, but we won't merge it if you think it might cause regressions.

This is how it will work with the changes.

const config1 = Config.string();

const config2 = config1.required();

console.log(config1.config.required); // undefined
console.log(config1.config.validator(2)); // returns error (not string)

console.log(config2.config.required); // true
console.log(config2.config.validator(2)); // returns error (not string)

[bryceosterhaus]

Yeah this should be fine. I don't see any issue with it.

[robframpton]

Just to follow up on this. An alternative solution is to provide a helper method that will create a new config object based on a predefined one. That way we don't accidentally break someone's implementation. Here is what I'm thinking.

const config1 = Config.string();

const config2 = Config.clone(config1).required();
// Or maybe Config.create ?

console.log(config1.config.required); // undefined
console.log(config2.config.required); // true

@carloslancha that would work for your use case right?

[carloslancha]

Hey @Robert-Frampton that would definitely work for me but I prefer the first approach (cloned by default). I think is safer and in this kind of objects I can't think in a case where you want to share mutable objects to configure states.

Thx!!

[mthadley]

Making the API more "immutable" should be safe. I'd be very surprised if someone was somehow relying on the mutation...

[robframpton]

Sounds good to me. @jbalsas if you're good with this #311 is ready for merging.

[jbalsas]

Merged to develop!

[carloslancha]

Thanks @Robert-Frampton !