-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: Add support for user nameGenerator
function
#4773
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
base: main
Are you sure you want to change the base?
Conversation
segments: PathSegment[] = [], | ||
experimental_customMergeAllOf?: Experimental_CustomMergeAllOf<S>, |
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.
Putting it at the end is a better choice to avoid a breaking change here
segments: PathSegment[] = [], | |
experimental_customMergeAllOf?: Experimental_CustomMergeAllOf<S>, | |
experimental_customMergeAllOf?: Experimental_CustomMergeAllOf<S>, | |
segments: PathSegment[] = [], |
* @param [segments=[]] - The path segments array to build upon | ||
* @param [experimental_customMergeAllOf] - Optional function that allows for custom merging of `allOf` schemas |
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.
* @param [segments=[]] - The path segments array to build upon | |
* @param [experimental_customMergeAllOf] - Optional function that allows for custom merging of `allOf` schemas | |
* @param [experimental_customMergeAllOf] - Optional function that allows for custom merging of `allOf` schemas | |
* @param [segments=[]] - The path segments array to build upon |
pathSchema={(() => { | ||
try { | ||
return isObject(schema) ? registry.schemaUtils.toPathSchema(schema, '', formData) : undefined; | ||
} catch { | ||
// Silently handle malformed schemas in tests | ||
return undefined; | ||
} | ||
})()} |
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 this is a one-shot call in the root Form
just compute it first into a local variable above and then use that variable here
/** Optional root name to use with nameGenerator | ||
* @default "root" | ||
*/ | ||
rootName?: string; |
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.
If you are required to pass the nameGenerator implementation in, why not just have that generator know what it wants to use for the root
. This will avoid an unnecessary prop.
I noticed that you've provided some sample implementations of these in utils
. If someone wanted to just use them straight up, but with a different rootName
they could wrap them?
} | ||
|
||
/** Function to generate HTML name attributes from path segments */ | ||
export type NameGeneratorFunction = (segments: PathSegment[], rootName?: string) => string | undefined; |
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.
No need for the rootName
parameter since you can implement the name generator however you want. Thus you can hard-code in the rootName
there.
formContext: { | ||
...(this.props.formContext || formContext), | ||
...(this.props.nameGenerator && { | ||
nameGenerator: this.props.nameGenerator, | ||
rootName: this.props.rootName || 'root', | ||
}), | ||
}, |
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.
We are about to update Registry
with a globalOptions?: GlobalFormOptions
which is designed to contain Form
related option that must be pass along to everything. Consider adding it to there instead. If you'd rather move forward faster, feel free to initially add the prop to the Registry
// Create path schema for array item with segments | ||
const itemPathSegments: PathSegment[] = [...(pathSchema?.$segments || []), { type: 'array', key: index }]; | ||
|
||
const itemPathSchema: PathSchema<T> = { | ||
$name: `${pathSchema?.$name || ''}.${index}`, | ||
$segments: itemPathSegments, | ||
}; | ||
|
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.
This all seems like a lot of extra work, just for the situation where someone MIGHT be using a nameGenerator
. I wonder if there is a way to make this whole feature be a bit more of a plug-in rather than a hard-wire? Perhaps leveraging the registry
to wrap the whole concept, with the pathSchema
and all into a new getHtmlName(registry)
function... Nah that gets more challenging.
Since one of your generators basically is designed to generate a dotted path name/id which is already covered by using the idSeparator
flag set to '.'
, all you are wanting to accomplish is changing the id to switch the index separator from idSeparator
to []
. Which takes me back to my original suggestion of adding a new option to toIdSchema()
to accomplish that, especially since toIdSchema()
is only called in the following places:
SchemaField on line 136,
LayoutGridField on line 507,
ArrayField on lines 789 and 535,
Form on 527
Especially since we are considering making the idPrefix
and idSeparator
be part of the GlobalFormOptions
in the registry
here so you would only have to update 5 places. rather than do more wiring.
@nlemoine I guess the TLDR is I'm not a fan of the approach, as it is too complex for your use case. I am open to you arguing your case for this approach over a simple flag to update the |
Hey @heath-freenome, thanks for the thorough review! I really appreciate your time and feedback. I understand your comments and will try to address your concerns, particularly about using Backend need examples
The
|
const itemIdSchema = schemaUtils.toIdSchema(itemSchema, itemIdPrefix, itemCast, idPrefix, idSeparator); | |
// Compute the item UI schema using the helper method | |
const itemUiSchema = this.computeItemUiSchema(uiSchema, item, index, formContext); | |
return this.renderArrayFieldItem({ | |
key, | |
index, | |
name: name && `${name}-${index}`, |
I gave a quick and dirty try to that approach and it gave me:
<input id="root[listOfStrings]_0" name="root[listOfStrings]_0" class="form-control" label="listOfStrings-0" required="" placeholder="" type="text" aria-describedby="root[listOfStrings]_0__error root[listOfStrings]_0__description root[listOfStrings]_0__help" value="foo">
But most importantly, it would bring some more major issues, IMHO.
HTML Requires different IDs and names
Even with a toIdSchema
working solution (e.g. generating bracketted ids), this isn't just about backend preferences - HTML itself requires different values for id
and name
attributes in several fundamental cases:
1. Radio button groups
Radio buttons in the same group must share the same name
but have different id
values:
<!-- ✅ Same name, different IDs -->
<input type="radio" id="root_color_0" name="root[color]" value="red">
<label for="root_color_0">Red</label>
<input type="radio" id="root_color_1" name="root[color]" value="blue">
<label for="root_color_1">Blue</label>
<!-- ❌ Using ID as name breaks radio grouping -->
<input type="radio" id="root_color_0" name="root_color_0" value="red">
<input type="radio" id="root_color_1" name="root_color_1" value="blue">
Radio group is already ok in current RJSF state but if id
is used as name
, this will no longer be the case.
2. Checkbox arrays
Multiple checkboxes that submit as an array need the same base name:
<!-- ✅ PHP-style array submission -->
<input type="checkbox" id="root_hobbies_0" name="root[hobbies][]" value="reading">
<input type="checkbox" id="root_hobbies_1" name="root[hobbies][]" value="gaming">
<input type="checkbox" id="root_hobbies_2" name="root[hobbies][]" value="cooking">
<!-- Submits as: hobbies = ['reading', 'gaming'] when multiple are checked -->
<!-- ❌ Using ID as name -->
<input type="checkbox" id="root_hobbies_0" name="root_hobbies_0" value="reading">
<input type="checkbox" id="root_hobbies_1" name="root_hobbies_1" value="gaming">
<!-- Submits as separate unrelated values, not an array! -->
Same as above.
3. Select multiple
Similar issue with multi-select dropdowns:
<!-- Needs array notation in name -->
<select id="root_skills" name="root[skills][]" multiple>
<option value="js">JavaScript</option>
<option value="python">Python</option>
</select>
Why brackets in IDs are complicated
Even if we wanted to use the same bracketed format for both ID and name (like root[tasks][0]
), it would break or weaken major parts of the web stack:
CSS selectors
/* Brackets are special characters in CSS */
#root[tasks][0] { } /* ❌ BREAKS - interpreted as attribute selector */
/* Would require escaping everywhere */
#root\[tasks\]\[0\] { } /* Ugly and error-prone */
/* Current RJSF approach */
#root_tasks_0 { } /* ✅ Clean, simple, works everywhere */
JavaScript DOM Queries
// querySelector fails with special characters
document.querySelector('#root[tasks][0]') // ❌ SyntaxError
// Would need escaping
document.querySelector('#root\\[tasks\\]\\[0\\]') // Complex escaping
// Current clean approach
document.querySelector('#root_tasks_0') // ✅ Works everywhere
I would have loved a much simpler solution too. Adding a flag in toIdSchema
feels like a workaround more than a robust and flexible alternative.
I tried to think of a one that brings the minimal possible changes. It can surely be improved (JS/React is not my strong suit) and I will address your review comments if you change your mind.
However, if you are still not convinced and don't see any benefits in bringing separate logic for id
and name
attributes, I would totally understand. I'm ready to work on a toIdSchema
alternative, since I'd really like that feature to make it for version 6.
What do you think? I'll be happy to refactor the implementation based on your preferences.
@nlemoine Thank you for the well documented explanation. It has convinced me and my compatriot @nickgros that there is merit in your approach. So much so, that I've just pushed a PR that does a major breaking refactor of the system to simplify your work quite a bit. In order to implement your feature, I am going to recommend the following approach after my PR merges:
|
Reasons for making this change
Fixes #4693
If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number]
(ex:fixes #123
).Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.