Refactor idSchema related code #950
Conversation
| $id: "rjsf", | ||
| foo: { $id: "rjsf_foo" }, | ||
| bar: { $id: "rjsf_bar" }, | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@edi9999 Is there a reason you need idPrefix instead of putting rootFieldId in the uiSchema?
| formData[i] | ||
| ); | ||
| } | ||
| // exception: formData is longer the the given length |
There was a problem hiding this comment.
Do we have a preferred behavior for this case?
There was a problem hiding this comment.
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.
|
Ideally passing a |
|
closing this as it's too old and no longer the best solution in my opinion. {
foo: { bar_baz: { $id: 'foo_bar_baz' } },
foo_bar: { baz: { $id: 'foo_bar_baz' } },
}but this doesn't solve the issue entirely the ideal solution IMO is to use json pointer or secondly, escape the separator character (e.g. |
Reasons for making this change
Simply refactoring, no functionality change.
The reasons is to generate a well formed idSchema at the root instead of re-generate it everytime in the children components.
Also with this change, users can customize idSchema easier.
!!!This change might potentially break custom array schema components rely on idSchema.!!!
Checklist
npm run cs-formaton my branch to conform my code to prettier coding style