-
-
Notifications
You must be signed in to change notification settings - Fork 512
fix(form-core): prevent duplicate dots when concatenating withFieldGroup
paths
#1639
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
View your CI Pipeline Execution ↗ for commit d862d99
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1639 +/- ##
==========================================
+ Coverage 90.37% 90.38% +0.01%
==========================================
Files 36 36
Lines 1621 1623 +2
Branches 385 386 +1
==========================================
+ Hits 1465 1467 +2
Misses 139 139
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@stefan-huck Thanks for the report! I see the edge case, but how did you encounter this? I'll add a unit test for the new line shortly, unless you want to add one. Definitely don't want to regress on this. |
it('should not duplicate dots if the second path starts with one', () => {
expect(concatenatePaths('foo', '.bar')).toBe('foo.bar')
}) This should do the trick. No idea when this situation could happen though. |
"use client"
import { createFormHook, createFormHookContexts } from "@tanstack/react-form"
const { fieldContext, formContext } = createFormHookContexts()
export const { useAppForm, withForm, withFieldGroup } = createFormHook({
fieldComponents: {},
formComponents: {},
fieldContext,
formContext,
})
const ChildForm = withFieldGroup({
defaultValues: {} as {
foo: string
// The nested prop 'street' shares the same name as the parent prop street
address: { street: string }
},
render: ({ group }) => (
<group.AppForm>
<group.AppField name="address.street">
{field => {
console.info(field.name, field.state.value)
// customerAddress..street some-street-value (see double dots)
return null
}}
</group.AppField>
</group.AppForm>
),
})
export function ParentForm() {
const form = useAppForm({
defaultValues: {
customerAddress: { street: "some-street-value" },
foo: "bar",
},
})
return (
<form.AppForm>
<ChildForm
form={form}
fields={{
foo: "foo",
address: "customerAddress",
}}
/>
</form.AppForm>
)
} I needed some time to dig into this, as it only occurs under strange circumstances. When both the However, if you avoid using the same key So, I think the solution is to avoid using nested values inside withFieldGroups. What do you think? |
@stefan-huck Can you amend the unit test I provided so we don't regress on this? I'll merge once that's done. |
@LeCarbonator I am not sure if my fix helps. Having two nested Forms with different keys const ChildForm = withFieldGroup({
defaultValues: {} as {
foo: string
// Rename
address: { streetAlternative: string }
},
...
export function ParentForm() {
const form = useAppForm({
defaultValues: {
street: "some-street-value",
foo: "bar",
},
})
return (
<form.AppForm>
<ChildForm
form={form}
fields={{
foo: "foo",
address: "address.street",
}}
/>
</form.AppForm>
)
} Results in address: never
Imo nested values in withFieldGroups doesn't work as expected. I would expect
|
@stefan-huck yes, the field map is shallow. It doesn't recursively check nested values. The reasoning for that was that records and arrays cannot be mapped with this kind of structure, which already blocks them from being used at top level. On top of that, if you have a group of fields with the intent to map them to different keys, then you don't need namespaces such as the address object. address.street in a field group may as well be an arbitrary name called addressStreet if you plan on remapping it to a different path. However, if you have address objects as compound field, then you expect the form to map to that object value (or something that satisfies it). |
withFieldGroup
paths
Thanks a lot for the quick fix! |
No description provided.