Skip to content

Refactor idSchema related code #950

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

Closed
knilink wants to merge 1 commit into from
Closed

Refactor idSchema related code #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 14 additions & 6 deletions src/components/Form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export default class Form extends Component {
const mustValidate = edit && !props.noValidate && liveValidate;
const { definitions } = schema;
const formData = getDefaultFormState(schema, props.formData, definitions);
const retrievedSchema = retrieveSchema(schema, definitions, formData);

const { errors, errorSchema } = mustValidate
? this.validate(formData, schema)
Expand All @@ -49,11 +48,10 @@ export default class Form extends Component {
errorSchema: state.errorSchema || {},
};
const idSchema = toIdSchema(
retrievedSchema,
uiSchema["ui:rootFieldId"],
schema,
uiSchema["ui:rootFieldId"] || props.idPrefix,
definitions,
formData,
props.idPrefix
formData
);
return {
schema,
Expand Down Expand Up @@ -102,7 +100,17 @@ export default class Form extends Component {

onChange = (formData, newErrorSchema) => {
const mustValidate = !this.props.noValidate && this.props.liveValidate;
let state = { formData };
const { schema, uiSchema } = this.state;
const { definitions } = schema;
let state = {
formData,
idSchema: toIdSchema(
schema,
uiSchema["ui:rootFieldId"] || this.props.idPrefix,
definitions,
formData
),
};
if (mustValidate) {
const { errors, errorSchema } = this.validate(formData);
state = { ...state, errors, errorSchema };
Expand Down
23 changes: 3 additions & 20 deletions src/components/fields/ArrayField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
allowAdditionalItems,
optionsList,
retrieveSchema,
toIdSchema,
getDefaultRegistry,
} from "../../utils";

Expand Down Expand Up @@ -368,7 +367,6 @@ class ArrayField extends Component {
registry = getDefaultRegistry(),
onBlur,
onFocus,
idPrefix,
rawErrors,
} = this.props;
const title = schema.title === undefined ? name : schema.title;
Expand All @@ -380,14 +378,7 @@ class ArrayField extends Component {
items: formData.map((item, index) => {
const itemSchema = retrieveSchema(schema.items, definitions, item);
const itemErrorSchema = errorSchema ? errorSchema[index] : undefined;
const itemIdPrefix = idSchema.$id + "_" + index;
const itemIdSchema = toIdSchema(
itemSchema,
itemIdPrefix,
definitions,
item,
idPrefix
);
const itemIdSchema = idSchema[index];
return this.renderArrayFieldItem({
index,
canMoveUp: index > 0,
Expand Down Expand Up @@ -510,7 +501,6 @@ class ArrayField extends Component {
uiSchema,
formData,
errorSchema,
idPrefix,
idSchema,
name,
required,
Expand Down Expand Up @@ -551,14 +541,7 @@ class ArrayField extends Component {
const itemSchema = additional
? retrieveSchema(schema.additionalItems, definitions, item)
: itemSchemas[index];
const itemIdPrefix = idSchema.$id + "_" + index;
const itemIdSchema = toIdSchema(
itemSchema,
itemIdPrefix,
definitions,
item,
idPrefix
);
const itemIdSchema = idSchema[index];
const itemUiSchema = additional
? uiSchema.additionalItems || {}
: Array.isArray(uiSchema.items)
Expand Down Expand Up @@ -592,7 +575,7 @@ class ArrayField extends Component {
rawErrors,
};

// Check if a custom template template was passed in
// check if a custom template template was passed in
const Template = ArrayFieldTemplate || DefaultFixedArrayFieldTemplate;
return <Template {...arrayProps} />;
}
Expand Down
8 changes: 0 additions & 8 deletions src/components/fields/SchemaField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ import PropTypes from "prop-types";
import {
isMultiSelect,
retrieveSchema,
toIdSchema,
getDefaultRegistry,
mergeObjects,
getUiOptions,
isFilesArray,
deepEquals,
Expand Down Expand Up @@ -155,7 +153,6 @@ function SchemaFieldRender(props) {
uiSchema,
formData,
errorSchema,
idPrefix,
name,
required,
registry = getDefaultRegistry(),
Expand All @@ -168,10 +165,6 @@ function SchemaFieldRender(props) {
} = registry;
let idSchema = props.idSchema;
const schema = retrieveSchema(props.schema, definitions, formData);
idSchema = mergeObjects(
toIdSchema(schema, null, definitions, formData, idPrefix),
idSchema
);
const FieldComponent = getFieldComponent(schema, uiSchema, idSchema, fields);
const { DescriptionField } = fields;
const disabled = Boolean(props.disabled || uiSchema["ui:disabled"]);
Expand Down Expand Up @@ -206,7 +199,6 @@ function SchemaFieldRender(props) {
const field = (
<FieldComponent
{...props}
idSchema={idSchema}
schema={schema}
uiSchema={{ ...uiSchema, classNames: undefined }}
disabled={disabled}
Expand Down
53 changes: 38 additions & 15 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,35 +619,58 @@ export function shouldRender(comp, nextProps, nextState) {
return !deepEquals(props, nextProps) || !deepEquals(state, nextState);
}

export function toIdSchema(
schema,
id,
definitions,
formData = {},
idPrefix = "root"
) {
export function toIdSchema(schema, id, definitions, formData, seperator = "_") {
const idSchema = {
$id: id || idPrefix,
$id: id || "root",
};
if ("$ref" in schema) {
if ("$ref" in schema || "dependencies" in schema) {
const _schema = retrieveSchema(schema, definitions, formData);
return toIdSchema(_schema, id, definitions, formData, idPrefix);
}
if ("items" in schema && !schema.items.$ref) {
return toIdSchema(schema.items, id, definitions, formData, idPrefix);
return toIdSchema(_schema, id, definitions, formData, seperator);
}
if ("items" in schema) {
for (const i in formData) {
const itemId = idSchema.$id + seperator + i;
if (Array.isArray(schema.items)) {
if (i < schema.items.length) {
idSchema[i] = toIdSchema(
schema.items[i],
itemId,
definitions,
formData[i]
);
} else if (schema.additionalItems) {
idSchema[i] = toIdSchema(
schema.additionalItems,
itemId,
definitions,
formData[i]
);
}
// exception: formData is longer the the given length
Copy link
Copy Markdown
Contributor

@glasserc glasserc Sep 26, 2018

Choose a reason for hiding this comment

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

Do we have a preferred behavior for this case?

Copy link
Copy Markdown
Contributor Author

@knilink knilink Oct 1, 2018

Choose a reason for hiding this comment

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

This happen when the schema is a fix length array but the form data is longer then it. If those exceed are not go be displayed on the form then it should be fine not assigning an id.

} else {
idSchema[i] = toIdSchema(
schema.items,
itemId,
definitions,
formData[i]
);
}
}
return idSchema;
}
if (schema.type !== "object") {
return idSchema;
}
formData = formData || {};
for (const name in schema.properties || {}) {
const field = schema.properties[name];
const fieldId = idSchema.$id + "_" + name;
const fieldId = idSchema.$id + seperator + name;
idSchema[name] = toIdSchema(
field,
fieldId,
definitions,
formData[name],
idPrefix
seperator
);
}
return idSchema;
Expand Down
22 changes: 13 additions & 9 deletions test/utils_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,9 +1027,15 @@ describe("utils", () => {
},
};

expect(toIdSchema(schema)).eql({
expect(
toIdSchema(schema, undefined, undefined, [
{ foo: "foo 0" },
{ foo: "foo 1" },
])
).eql({
$id: "root",
foo: { $id: "root_foo" },
0: { $id: "root_0", foo: { $id: "root_0_foo" } },
1: { $id: "root_1", foo: { $id: "root_1_foo" } },
});
});

Expand Down Expand Up @@ -1068,13 +1074,11 @@ describe("utils", () => {
$ref: "#/definitions/testdef",
};

expect(toIdSchema(schema, undefined, schema.definitions, {}, "rjsf")).eql(
{
$id: "rjsf",
foo: { $id: "rjsf_foo" },
bar: { $id: "rjsf_bar" },
}
);
expect(toIdSchema(schema, "rjsf", schema.definitions, {})).eql({
$id: "rjsf",
foo: { $id: "rjsf_foo" },
bar: { $id: "rjsf_bar" },
});
Copy link
Copy Markdown
Contributor

@glasserc glasserc Sep 26, 2018

Choose a reason for hiding this comment

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

Previously ui:rootFieldId on the root element was not passed on to children; now I think it is. Is that intentional? This test almost seems like it was meant to ensure that behavior, but it's a bit hard to say for sure.

Assuming we don't care about preserving the idPrefix behavior, this section is called "should handle idPrefix parameter", but there is no longer such a parameter, so maybe we should delete the whole thing.

Copy link
Copy Markdown
Contributor Author

@knilink knilink Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This code here in the master branch
https://github.com/mozilla-services/react-jsonschema-form/blob/7c252de331cfa77c415adf49c04eeb25fd91d28b/src/utils.js#L621-L630
where id=uiSchema["ui:rootFieldId"] as shown bellow.
https://github.com/mozilla-services/react-jsonschema-form/blob/7c252de331cfa77c415adf49c04eeb25fd91d28b/src/components/Form.js#L65-L71
So, no matter what, the fact is ui:rootFieldId and idPerfix are doing the same thing.

About passing down

Previously, toIdSchema didn't pass down to array I guess it's because it relied on array data, which was not given to toIdSchema in the very beginning. But now, since the toIdSchema has the form data, it should be able to generate a well formed idSchema at the root instead of being called again in ObjectField and ArrayField.

About keeping idPrefix

I didn't remove idPrefix for compatibility reason because it's documented and someone might have been using it. If we decide to clean it up, it's fine to delete the whole thing.

Copy link
Copy Markdown
Contributor

@glasserc glasserc Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Wait, now I'm even more confused. I definitely misread the code at first, and you're right, it seems like ui:rootFieldId and idPrefix are doing the same thing, but we're not maintaining compatibility with idPrefix -- any idPrefix is going to be ignored completely.

I'm the one who merged the PR to add idPrefix in #806. I should have paid more attention and noticed that it duplicates the rootFieldId option! But it's definitely documented and I bet @edi9999 uses it somewhere. We should either make an effort to maintain backwards compatibility or admit defeat and add it to #805. What do you think?

Copy link
Copy Markdown
Collaborator

@edi9999 edi9999 Oct 2, 2018

Choose a reason for hiding this comment

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

Yes, I am indeed using idPrefix to uniquely identify form elements from our tests. It could be good if you keep backwards compatibility, but I would understand if you don't.

Copy link
Copy Markdown
Contributor

@glasserc glasserc Oct 2, 2018

Choose a reason for hiding this comment

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

@edi9999 Is there a reason you need idPrefix instead of putting rootFieldId in the uiSchema?

});
});

Expand Down