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.

Sign up for GitHub

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

Fix for #227 #233

Merged
merged 2 commits into from
May 4, 2017
Merged

Fix for #227 #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0a307a
Failing test case for issue #227
bryceosterhaus May 3, 2017
44eeda4
Fixes #227
bryceosterhaus May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 76 additions & 6 deletions packages/metal-state/src/Config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,82 @@ function mergeConfig(context, config) {
return obj;
}

// Add all validators to `Config`.
const fnNames = Object.keys(validators);
fnNames.forEach(
name => Config[name] = function() {
/**
* Adds primitive type validators to the config object.
* @param {string} name The name of the validator.
*/
function setPrimitiveValidators(name) {
Config[name] = function() {
return this.validator(validators[name]);
}
);
};
}

setPrimitiveValidators('any');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to make these (and shapeOf, oneOfType, etc.) just be methods on the Config as defined above? As oppose to declaring the object, and then mutating it down here over and over.

The implementation of each method could still just call some helper for the behavior that is shared.

Another idea would be to declare another object, something like ConfigValidators, and just merge the two objects at the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless this proposal the end behavior should be the same, correct? If so this pull looks good to me and then if you want you can send a refactor for the internals.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be the same.

It would just make this file much easier to read, since all methods on Config would be explicitly declared in the same way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Merging this one and refactoring is welcome. Thx.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I will go ahead and refactor and send another PR.

setPrimitiveValidators('array');
setPrimitiveValidators('bool');
setPrimitiveValidators('func');
setPrimitiveValidators('number');
setPrimitiveValidators('object');
setPrimitiveValidators('string');

/**
* Calls validators with a single nested config.
* @param {string} name The name of the validator.
*/
function setNestedValidators(name) {
Config[name] = function(arg) {
return this.validator(validators[name](arg.config.validator));
};
}

setNestedValidators('arrayOf');
setNestedValidators('objectOf');

/**
* Calls validators with provided argument.
* @param {string} name The name of the validator.
*/
function setExplicitValueValidators(name) {
Config[name] = function(arg) {
return this.validator(validators[name](arg));
};
}

setExplicitValueValidators('instanceOf');
setExplicitValueValidators('oneOf');

Config.oneOfType = function(validatorArray) {
validatorArray = validatorArray.map(
item => item.config.validator
);

return this.validator(validators.oneOfType(validatorArray));
};

/**
* Recursively sets validators for shapeOf.
* @param {!Object} shape The shape of specific types.
*/
function destructShapeOfConfigs(shape) {
const keys = Object.keys(shape);

const retShape = {};

keys.forEach(
key => {
const value = shape[key];

retShape[key] = value.config && value.config.validator ? value.config.validator : destructShapeOfConfigs(value);
}
);

return retShape;
}

Config.shapeOf = function(shapeObj) {
shapeObj = destructShapeOfConfigs(shapeObj);

return this.validator(validators.shapeOf(shapeObj));
};

export default Config;
80 changes: 79 additions & 1 deletion packages/metal-state/test/Config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,92 @@ describe('Config', function() {
}, config.config);
});

it('should return config with specific validator from "validators"', function() {
it('should return config with "arrayOf" validator from "validators"', function() {
var config = Config.arrayOf(Config.number());
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator([1, 2]));
assert.ok(config.config.validator([1, 'one']) instanceof Error);
assert.ok(config.config.validator(['one']) instanceof Error);
});

it('should return config with "instanceOf" validator from "validators"', function() {
class TestClass {
}
class TestClass2 {
}

var config = Config.instanceOf(TestClass);
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator(new TestClass()));
assert.ok(config.config.validator(new TestClass2()) instanceof Error);
});

it('should return config with "number" validator from "validators"', function() {
var config = Config.number();
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator(10));
assert.ok(config.config.validator('test') instanceof Error);
});

it('should return config with "oneOf" validator from "validators"', function() {
var config = Config.oneOf([1, 'one']);
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator(1));
assert.ok(config.config.validator('one'));
assert.ok(config.config.validator(2) instanceof Error);
assert.ok(config.config.validator(false) instanceof Error);
});

it('should return config with "shapeOf" validator from "validators"', function() {
var shape = {
one: Config.string(),
two: {
three: {
four: Config.number()
}
},
five: Config.arrayOf(Config.string())
};

var pass = {
one: 'one',
two: {
three: {
four: 4
}
},
five: ['five']
};

var fail = {
one: 'one',
two: {
three: {
four: 'four'
}
},
five: 5
};

var config = Config.arrayOf(Config.shapeOf(shape));
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator(pass));
assert.ok(config.config.validator(fail) instanceof Error);
});

it('should return config with "string" validator from "validators"', function() {
var config = Config.string();
assert.ok(core.isObject(config));
assert.ok(core.isFunction(config.config.validator));
assert.ok(config.config.validator('test'));
assert.ok(config.config.validator(10) instanceof Error);
});

it('should return config with data from multiple calls', function() {
var setter = () => {
};
Expand Down