fix: MultiSchema getting incorrect id in arrays #2199
Conversation
|
Hey @QingqiShi , sorry for the delay in reviewing. Is this PR essentially the same as the earlier PR #2127? |
|
@epicfaace Yes! I accidentally force pushed the other branch which destroyed the PR, this one is identical. |
QingqiShi
force-pushed
the
fix/multi-schema-id
branch
from
5bc6055 to
6e83df1
Compare
|
Need some help with this one. Build is failing after rebasing to the latest |
|
This is technically a breaking change (though correct behavior), and so it can go into the 3.x release. @QingqiShi can you add a section to the "3.x upgrade guide" in the documentation that describes what changed and what users should watch out for? |
|
@QingqiShi It looks like the failing test is happening because the old test is specifically depending on the previous behavior, that is, four dropdowns with the same id in arrays. Can you update the test so it works with this PR's changes with respect to ids? |
|
@epicfaace Thank you for pointing me to the failing test! The failing test is now fixed, and I've added a section to the upgrade guide as you've suggested. Thanks! |
| @@ -243,7 +243,7 @@ function SchemaFieldRender(props) { | |||
| let idSchema = props.idSchema; | |||
| const schema = retrieveSchema(props.schema, rootSchema, formData); | |||
| idSchema = mergeObjects( | |||
| toIdSchema(schema, null, rootSchema, formData, idPrefix), | |||
| toIdSchema(schema, idSchema.$id, rootSchema, formData, idPrefix), | |||
There was a problem hiding this comment.
I'm a bit confused as to what exactly this change does. Do you mind explaining a bit as to how this change in the logic fixes the issue? @QingqiShi
Depending on how complex it is, it might be good for us to add a comment so future users of this code can more easily understand, as well!
There was a problem hiding this comment.
Gladly.
However I'm not sure if we need much explaining here. The call signature for toIdSchema is toIdSchema(schema, id, rootSchema, formData, idPrefix), so I believe passing null to the id argument is incorrect to begin with.
More specifically, in the ArrayField each item gets an itemIdSchema with the id idSchema.$id + "_" + index, this id then gets passed to each item rendered bySchemaField. The logic after that is a bit fuzzy to me, but I imagine if we don't use it during the generation of new ids, it defaults to 'root' prefix and this wrong id eventually get passed to the MultiSchema component.
🙏 Thanks again for spending the time to maintain this package.
There was a problem hiding this comment.
I see, thanks for the explanation! I'm still a bit confused as to how this change would have only affected the ids in arrays in MultiSchemaField. I would have expected that such a change to SchemaField.js would have had a significant effect on all / many more ids generated through idSchema. Do you know why that's not the case?
|
@QingqiShi So I'd love to get this change into the upcoming v5 beta... do you have the bandwidth to port this to the |
- Replicated the fix made in rjsf-team#2199 since it is a breaking change and we are doing a bunch of them
- Replicated the fix made in rjsf-team#2199 since it is a breaking change and we are doing a bunch of them - Also updated the `CHANGELOG.md`
- Replicated the fix made in rjsf-team#2199 since it is a breaking change and we are doing a bunch of them - Also updated the `CHANGELOG.md`
|
Fixed in v5 beta via #3039 |
Fixes #2197
Reasons for making this change
When MultiSchemaField is part of an array the idSchema is incorrectly generated. This commit fixes that by making sure the parent id is passed to
toIdSchema.#2197
Checklist