-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix #4733 path duplication #4735
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
Conversation
I stumbled across the same issue today. @Fronix I think your fix could cause unwanted side effects. When I have an object with a property "prop" which again is an object with a property "prop", your fix would make it impossible to change the value for "prop.prop". At least that's my understanding of the method. While debugging the issue, I identified the ArrayField.onSelectChange method to be problematic:
If you look at other methods of ArrayField, you can see that an empty path array is provided to onChange with a comment that the appropriate path is added by onChange:
I'd say onSelectChange needs to do the same (pass an empty path array to onChange). However, I don't know if this would break other use cases ... |
That is true, I didn't think of the case of Changing to this /** Callback handler used to change the value for a checkbox */
onSelectChange = (value: any) => {
const { onChange, idSchema } = this.props;
// To ensure it's possible to change prop.prop.... by extracting the correct path from the idSchema.$id
const path = idSchema && idSchema.$id ? idSchema.$id.split('_').slice(1) : [];
onChange(value, path, undefined, idSchema && idSchema.$id);
}; and then keeping this would allow for nested properties, with the same name, to work if (!changePath.length || changePath[0] !== name) {
changePath.unshift(name);
} I could also just be very unfamiliar with the inner workings, and there might be a much better way to do it but without knowing the real path of the properties, it feels impossible to build the path as you go and then using unshift without breaking non nested objects. Edit: There can also be an argument to not support |
const changePath = Array.isArray(path) ? path.slice() : []; | ||
changePath.unshift(name); | ||
// Ensure we are not duplicating the path | ||
if (!changePath.length || changePath[0] !== name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is similar code in ArrayField
might I consider making a similar change there? AND there is one problem with this fix...if there were nested properties with the same name, this would cause problems with incorrect nesting.
Can you provide a codesandbox example where you are seeing this problem so that we can better understand why this failed for you.
I.e.
{
"type": "object",
"properties": {
"foo": {
"type": "object",
"properties": {
"foo": {
"type": "string",
}
}
}
}
}
import set from 'lodash/set'; | ||
import unset from 'lodash/unset'; | ||
import Markdown from 'markdown-to-jsx'; | ||
import { Component } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this back to the top? We prefer react imports to be first
Reasons for making this change
Fixed #4721 that broke multiselect fields onchange by duplicating the path with
slice()
This fixes #4733
Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.