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

added additional support #458

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ build
dist
lib

/*.iml
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't commit these to the project's .gitignore. If you find them helpful, you might add them to your $HOME/.config/git/ignore (per the gitignore man page).

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"dependencies": {
"jsonschema": "^1.0.2",
"lodash.topath": "^4.5.2",
"lodash.merge": "^4.5.2",
"setimmediate": "^1.0.5"
},
"devDependencies": {
Expand Down
27 changes: 27 additions & 0 deletions playground/samples/additionalProperties.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
module.exports = {
schema: {
"title": "A registration form",
"description": "A simple form example.",
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"name": {
"type": "string"
},
"devMap": {
"type": "object",
"additionalProperties": {
"type": "integer"
}
}
}
},
uiSchema: {},
formData: {
"name": "test",
"devMap": {
"key": 2,
"value": 12
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this example is quite confusing. additionalProperties makes the most sense for properties with unknown and arbitrary names. The names key and value sound like two very specific names with a predefined meaning, and specifically they sound like an attempt to try to encode some entity like {"5": 12}. ({5: 12} is, of course, invalid JSON, since all keys have to be strings.) I think this is especially misleading because of the name devMap.

How about an example where the keys are really arbitrary, for example site names or URLs? I guess we'd still want the values to be numeric, so maybe it could be a map of site -> estimated user count or something?

}
}
};
2 changes: 2 additions & 0 deletions playground/samples/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import validation from "./validation";
import files from "./files";
import single from "./single";
import customArray from "./customArray";
import additionalProperties from "./additionalProperties";

export const samples = {
Simple: simple,
Expand All @@ -30,4 +31,5 @@ export const samples = {
Files: files,
Single: single,
"Custom Array": customArray,
AdditionalProperties: additionalProperties
};
4 changes: 2 additions & 2 deletions src/components/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export default class Form extends Component {
liveValidate: false,
safeRenderCompletion: false,
noHtml5Validate: false
}
};

constructor(props) {
super(props);
Expand Down Expand Up @@ -93,7 +93,7 @@ export default class Form extends Component {
if (this.props.onBlur) {
this.props.onBlur(...args);
}
}
};

onSubmit = (event) => {
event.preventDefault();
Expand Down
166 changes: 110 additions & 56 deletions src/components/fields/ObjectField.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@ function objectKeysHaveChanged(formData, state) {
return true;
}
// deep check on sorted keys
if (!deepEquals(newKeys.sort(), oldKeys.sort())) {
return true;
}
return false;
return !deepEquals(newKeys.sort(), oldKeys.sort());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not commit stylistic changes as part of this PR, it makes it harder to review.

}

class ObjectField extends Component {
Expand All @@ -36,7 +33,7 @@ class ObjectField extends Component {
required: false,
disabled: false,
readonly: false,
}
};

constructor(props) {
super(props);
Expand Down Expand Up @@ -84,67 +81,124 @@ class ObjectField extends Component {
};

render() {
const {
uiSchema,
errorSchema,
idSchema,
name,
required,
disabled,
readonly,
onBlur
} = this.props;
const {definitions, fields, formContext} = this.props.registry;
const {SchemaField, TitleField, DescriptionField} = fields;
const {name} = this.props;
const {definitions} = this.props.registry;
const schema = retrieveSchema(this.props.schema, definitions);
const title = (schema.title === undefined) ? name : schema.title;
if ("additionalProperties" in schema){
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Space before {

return this.renderAdditionalProperties(schema, title);
}

return this.renderProperties(schema, title);
}

renderProperties(schema, title){
const {
uiSchema,
errorSchema,
idSchema,
name,
required,
disabled,
readonly,
onBlur
} = this.props;
const {fields, formContext} = this.props.registry;
const {SchemaField, TitleField, DescriptionField} = fields;

let orderedProperties;
try {
const properties = Object.keys(schema.properties);
orderedProperties = orderProperties(properties, uiSchema["ui:order"]);
} catch (err) {
return (
<div>
<p className="config-error" style={{color: "red"}}>
Invalid {name || "root"} object field configuration:
<em>{err.message}</em>.
</p>
<pre>{JSON.stringify(schema)}</pre>
</div>
);
<div>
<p className="config-error" style={{color: "red"}}>
Invalid {name || "root"} object field configuration:
<em>{err.message}</em>.
</p>
<pre>{JSON.stringify(schema)}</pre>
</div>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not change indentation, here as well as elsewhere, as it no longer matches the style of the rest of the code, and the changes themselves are hard to review. I see that you unindented the code that eslint flagged, so clearly our eslint rules aren't strict enough about enforcing "house style", but please try to match it anyhow :)

}
return (
<fieldset>
{title ? <TitleField
id={`${idSchema.$id}__title`}
title={title}
required={required}
formContext={formContext}/> : null}
{schema.description ?
<DescriptionField
id={`${idSchema.$id}__description`}
description={schema.description}
formContext={formContext}/> : null}
{
orderedProperties.map((name, index) => {
return (
<SchemaField key={index}
name={name}
required={this.isRequired(name)}
schema={schema.properties[name]}
uiSchema={uiSchema[name]}
errorSchema={errorSchema[name]}
idSchema={idSchema[name]}
formData={this.state[name]}
onChange={this.onPropertyChange(name)}
onBlur={onBlur}
registry={this.props.registry}
disabled={disabled}
readonly={readonly}/>
);
})
}</fieldset>
);
<fieldset>
{title ? <TitleField
id={`${idSchema.$id}__title`}
title={title}
required={required}
formContext={formContext}/> : null}
{schema.description ?
<DescriptionField
id={`${idSchema.$id}__description`}
description={schema.description}
formContext={formContext}/> : null}
{orderedProperties.map((name, index) => {
return (
<SchemaField key={index}
name={name}
required={this.isRequired(name)}
schema={schema.properties[name]}
uiSchema={uiSchema[name]}
errorSchema={errorSchema[name]}
idSchema={idSchema[name]}
formData={this.state[name]}
onChange={this.onPropertyChange(name)}
onBlur={onBlur}
registry={this.props.registry}
disabled={disabled}
readonly={readonly}/>
);
})}
</fieldset>
);
}

renderAdditionalProperties(schema, title){
const {
uiSchema,
errorSchema,
idSchema,
required,
disabled,
readonly,
onBlur
} = this.props;
const {fields, formContext} = this.props.registry;
const {SchemaField, TitleField, DescriptionField} = fields;

return (
<fieldset>
{title ? <TitleField
id={`${idSchema.$id}__title`}
title={title}
required={required}
formContext={formContext}/> : null}
{schema.description ?
<DescriptionField
id={`${idSchema.$id}__description`}
description={schema.description}
formContext={formContext}/> : null}
{Object.keys(this.state).map((name, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Of the ways that this is different from the existing method, I don't understand why we no longer respect ui:order here.

const childIdSchema = {"$id": `${idSchema.$id}__${name}`};
return (
<SchemaField key={index}
name={name}
required={this.isRequired(name)}
schema={schema.additionalProperties}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this doesn't correctly support a schema that has both properties and additionalProperties.

uiSchema={uiSchema[name]}
errorSchema={errorSchema[name]}
idSchema={childIdSchema}
formData={this.state[name]}
onChange={this.onPropertyChange(name)}
onBlur={onBlur}
registry={this.props.registry}
disabled={disabled}
readonly={readonly}/>
);
})}
</fieldset>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to copy and paste this entire function. There are more commonalities than differences in the two implementations, which means most parts are unchanged. Trying to review this is challenging because I have to read a whole new method rather than an enhanced version of the existing method.

}
}

Expand Down
10 changes: 7 additions & 3 deletions src/utils.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import merge from "lodash.merge";
import React from "react";
import "setimmediate";


const widgetMap = {
boolean: {
checkbox: "CheckboxWidget",
Expand Down Expand Up @@ -112,7 +112,7 @@ function computeDefaults(schema, parentDefaults, definitions={}) {
if (isObject(defaults) && isObject(schema.default)) {
// For object defaults, only override parent defaults that are defined in
// schema.default.
defaults = mergeObjects(defaults, schema.default);
defaults = merge(defaults, schema.default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching from the existing mergeObjects function to lodash.merge is its own topic for discussion and should not be part of this PR. It makes it seem like using lodash.merge is necessary to implement this feature, which as far as I can tell, it isn't.

} else if ("default" in schema) {
// Use schema defaults for this node.
defaults = schema.default;
Expand All @@ -129,6 +129,10 @@ function computeDefaults(schema, parentDefaults, definitions={}) {
}
// We need to recur for object schema inner default values.
if (schema.type === "object") {
if (!schema.properties){
return defaults;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change about?


return Object.keys(schema.properties).reduce((acc, key) => {
// Compute the defaults for this node, with the parent defaults we might
// have from a previous run: defaults[key].
Expand All @@ -150,7 +154,7 @@ export function getDefaultFormState(_schema, formData, definitions={}) {
return defaults;
}
if (isObject(formData)) { // Override schema defaults with form data.
return mergeObjects(defaults, formData);
return merge(defaults, formData);
}
return formData || defaults;
}
Expand Down
72 changes: 0 additions & 72 deletions test/utils_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
deepEquals,
getDefaultFormState,
isMultiSelect,
mergeObjects,
pad,
parseDateString,
retrieveSchema,
Expand Down Expand Up @@ -264,77 +263,6 @@ describe("utils", () => {
});
});

describe("mergeObjects()", () => {
it("should't mutate the provided objects", () => {
const obj1 = {a: 1};
mergeObjects(obj1, {b: 2});
expect(obj1).eql({a: 1});
});

it("should merge two one-level deep objects", () => {
expect(mergeObjects({a: 1}, {b: 2})).eql({a: 1, b: 2});
});

it("should override the first object with the values from the second", () => {
expect(mergeObjects({a: 1}, {a: 2})).eql({a: 2});
});

it("should recursively merge deeply nested objects", () => {
const obj1 = {
a: 1,
b: {
c: 3,
d: [1, 2, 3],
e: {f: {g: 1}}
},
c: 2
};
const obj2 = {
a: 1,
b: {
d: [3, 2, 1],
e: {f: {h: 2}},
g: 1
},
c: 3
};
const expected = {
a: 1,
b: {
c: 3,
d: [3, 2, 1],
e: {f: {g: 1, h: 2}},
g: 1
},
c: 3
};
expect(mergeObjects(obj1, obj2)).eql(expected);
});

describe("concatArrays option", () => {
it("should not concat arrays by default", () => {
const obj1 = {a: [1]};
const obj2 = {a: [2]};

expect(mergeObjects(obj1, obj2)).eql({a: [2]});
});

it("should concat arrays when concatArrays is true", () => {
const obj1 = {a: [1]};
const obj2 = {a: [2]};

expect(mergeObjects(obj1, obj2, true)).eql({a: [1, 2]});
});

it("should concat nested arrays when concatArrays is true", () => {
const obj1 = {a: {b: [1]}};
const obj2 = {a: {b: [2]}};

expect(mergeObjects(obj1, obj2, true)).eql({a: {b: [1, 2]}});
});
});
});

describe("retrieveSchema()", () => {
it("should 'resolve' a schema which contains definitions", () => {
const schema = {$ref: "#/definitions/address"};
Expand Down