feat(react-form): Add withFieldGroup
#1469
Conversation
|
View your CI Pipeline Execution ↗ for commit 8ce40fe.
☁️ Nx Cloud last updated this comment at |
|
Issues that could be addressed:
This has now been addressed. If onSubmitMeta is unset, any value will do. If it is set, you must match it exactly. |
While the previous separate implementation was compatible with AppForm, users wouldn't have been able to use any field/form components in the render itself. This commit allows them to do that, at the expense of not being compatible with useForm.
|
The unit tests should be reusable in case this isn't the desired approach. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1469 +/- ##
==========================================
+ Coverage 89.24% 90.10% +0.86%
==========================================
Files 31 33 +2
Lines 1432 1557 +125
Branches 366 380 +14
==========================================
+ Hits 1278 1403 +125
Misses 137 137
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Related PR: #1334 |
|
Something to consider:
|
This type will help with a lens API for forms so that only names leading to the subset data are allowed.
|
#1475 sounded like an interesting idea, so I'll try to tinker with that and come up with unit tests. The note at the top of the PR still applies. |
…k-form into form-group-api
|
Strange, the derived state from the form lens isn't updating ... Issue: React input unfocuses when changing input. Unsure why. |
There was a problem hiding this comment.
DUDE - this is awesome. I'm genuinely so excited about it. I know I've gushed about this API in our private maintainer's channel, but want to share that excitement here as well.
Outside of these 7 items I think there's one big one:
I want to change this API's name to fieldGroup, where the lens property becomes group and the withFormLens becomes withFieldGroup.
I think the proposed name change feels more genuine and less CS-focused.
| this.store = new Derived({ | ||
| deps: [this.form.store], | ||
| fn: ({ currDepVals }) => { | ||
| const currFormStore = currDepVals[0] | ||
| const { | ||
| errors: formErrors, | ||
| errorMap: formErrorMap, | ||
| values: _, | ||
| ...rest | ||
| } = currFormStore | ||
|
|
||
| const { errorMap: lensErrorMap, errors: lensErrors } = | ||
| this.form.getFieldMeta(this.name) ?? { | ||
| ...defaultFieldMeta, | ||
| } | ||
|
|
||
| return { | ||
| ...rest, | ||
| lensErrorMap, | ||
| lensErrors, | ||
| formErrorMap, | ||
| formErrors, | ||
| values: this.form.getFieldValue(this.name) as TLensData, | ||
| } | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This is a pretty broad design question that I don't know the answer to yet, so I want you and other @TanStack/form-maintainers to weigh in:
Should we:
- Keep the derived state inside of the
FormLensclass? - Register a
FormLensinstance to theFormclass and compute the derived state inside of theFormclass?
We currently do 2 for Fields, but lenses might be sufficiently different from what we want to do.
The reason I loosely lean towards 2 and why I bring it up at all:
The current implementation will cause more re-renders than needed to due lack of caching values from prevDepVals; a pattern we follow for fields (but is tricky to get right).
We can fix this in the current implementation, but feels strange to have fields and lenses behave differently in how they compute their derived state
There was a problem hiding this comment.
The thing is, the lens itself should ideally not exist. It should merely be a translator between fields and a form using some form of mapping.
With this in mind, i think I will remove most state values as they are not related to it and can be accessed differently (submissionAttempts getting fetched from group.form.store instead of directly)
However, values would be convenient to have derived somehow for easier useStore and Subscribe. I'm just not sure how to go about it.
There was a problem hiding this comment.
I would lean towards 2 myself, it feels odd to have a separate state outside the form. Though it seems we're all in agreement
There was a problem hiding this comment.
The revised version tries to keep as little store as possible in the field group, since it's not its true purpose. Thoughts? @harry-whorlow @crutchcorn
|
After some discussion with @LeCarbonator - we thought of a feature addition: const Parent = () => {
// Docs change: Prefix all `withFieldGroup` components with `FieldGroup` to avoid confusion
const FieldGroupPassword = withFieldGroup({
defaultValues: {
firstName: '',
lastName: '',
},
render: ({ group, title }) => {
// ...
},
})
const form = useAppForm({
...formOpts,
})
// This is actually pretty type-safe since we'll get an error if the shape of the `fields` doesn't match
// the form name type.
export const fields = {
"password": 'password',
"confirm_password": 'confirm_password',
} as const;
return (
<FieldGroupPassword
form={form}
{/* Rename the `name`*/}
{/* Either */}
fields="person"
{/* OR */}
fields={fields}
{/* Via an overload */}
/>
)
} |
…m into form-group-api
withFormLenswithFieldGroup
the method appears to be a helper function for form.reset, which is accessible from the field group API. There does not seem to be a good reason to port this method from FormApi.
|
@LeCarbonator I've been toying this PR (I know it's not done) because it's exactly what I need. I'm not an experienced developer by any means, but I am a product manager by trade and so I'm used to thinking about user experience or in this case developer experience. Can I suggest that instead a fields prop that either takes a string or an object of key value pairs where you map a form property to the group property, could we instead have a group prop and we pass the actual form object to a group prop or map the destination property to the form property directly? That could be more intuitive since we pass the form prop to the withForm hook and this would feel similar in experience. I don't know the feasibility with that and maintaining type safety, though. |
|
@MPiland could you provide a code snippet of what that structure may look like? It doesn't need to be feasible, just a snippet of what you envisioned the API to be |
users are able to do conditional rendering, but TS generally doesn't pick up on it. Therefore, while it is less type safe, it allows users to use field groups in more locations than previously possible.
|
@LeCarbonator I think there's a couple ways that could maybe work. The prop could be just named group that could either accept an object or an object of properties mapped to form properties. This would probably require a form subscribe. Alternatively, you could add a new option to the createFormHook called groupComponents. They could be called similar to field components, but their name prop could take a string or an object of key values similar to how the withFieldGroup works. Then it could use a useGroupContext. This is all completely brainstorming so it may not be possible. But you could call it like The current approach still works, though, and if that makes more sense to other users, I'm ok being told I'm wrong. I'm just throwing out some ideas. I really like this library. It took me a minute to get it, but once I did it's very intuitive. I just need this last group feature to be able to use it. |
|
@MPiland Alright, here's my thoughts on the snippets: <form.Subscribe select={(state) => state.values)}>
{(values) => {
<Auth group={values.auth} form={form} />
}
</form.Subscribe>I'm heavily against this implementation. The reason is that field groups aren't just displaying values, they map fields called within to the form outside. This would force it to be a one-way street which goes against a lot of the features other users want. As for /* Do you want granular control? */
<form.AppField name="firstName">
{field => <>
<label>First Name</label>
<field.TextInput />
<field.Feedback />
</>}
</form.AppField>
/* Or wrap it all in a component? */
<form.AppField name="firstName">
{field => <field.TextInput showFeedback label="First Name" />}
</form.AppField>Field groups don't really fit that category, as they're meant to do the work themselves. Once you have the field group, there's really only one way to use it, and that is like you showed: /* Combinations don't really exist. Either you use the field group or you don't */
<form.AppGroup name='auth'>
{({ AuthGroup }) => (
<AuthGroup {...props} />
)} />
</form.AppGroup>That's why I prefer the HOC implementation. Hopefully it's clear what I'm trying to convey. |
|
@LeCarbonator That all makes sense to me! I will defer to your expertise. I think the original way works as well. I appreciate you entertaining my thoughts! |
Closes #1296
Todos
This PR implements a variation of
withFormthat can be used to create form groups. This form group allows extendingdefaultValuesand has no expectations of form level validators.This distinguishes it both from
withFormas well as instantiations of forms.Here's an extract of the documentation:
Reusing groups of fields in multiple forms
Sometimes, a pair of fields are so closely related that it makes sense to group and reuse them — like the password example listed in the linked fields guide. Instead of repeating this logic across multiple forms, you can utilize the
withFieldGrouphigher-order component.Rewriting the passwords example using
withFieldGroupwould look like this:We can now use these grouped fields in any form that implements the default values:
Mapping field group values to a different field
You may want to keep the password fields on the top level of your form, or rename the properties for clarity. You can map field group values
to their true location by changing the
fieldproperty:Important
Due to TypeScript limitations, field mapping is only allowed for objects. You can use records or arrays at the top level of a field group,
but you will not be able to map the fields.
If you expect your fields to always be at the top level of your form, you can create a quick map
of your field groups using a helper function: