Strongly typed form values for Flow and Typescript #516
Conversation
| RenderableProps<FormSpyRenderProps> {} | ||
| export interface FormSpyProps<FormValues> | ||
| extends UseFormStateParams<FormValues>, | ||
| RenderableProps<FormSpyRenderProps<FormValues>> {} | ||
|
|
||
| export const Field: React.FC<FieldProps<any>>; | ||
| export const Form: React.FC<FormProps<object>>; |
There was a problem hiding this comment.
I was actually about to submit a PR very similar to this. I just implemented final-form at the place I work and love it. I just wish the types were a bit stronger. While this PR is very close to what I had the Form component is still just getting the object generic type passed to it.
I actually implemented something like this:
export const Form: <FormValues = object>(
props: FormProps<FormValues>
) => React.FunctionComponentElement<FormProps<FormValues>>;
This should allow typescript to infer the types based on the initialValues props passed to the form or you could set the generic to the form itself like so:
<Form<{foo: string, bar: number }> initialValues={initialValues} onSubmit={onSubmit}>
This similar to what is in #426, but I'm not using a class since it isn't one.
I also did the same to Field:
export const Field: <FieldValue = any, T extends HTMLElement = HTMLElement>(
props: FieldProps<FieldValue, T>
) => React.FunctionComponentElement<FieldProps<FieldValue, T>>;
This way I could pass the type FieldType to UseFieldConfig and then the initalValue prop and others on Field would be strongly typed:
<Field<string> name="foo" label="Foo" initialValue="bar" component={TextField} required validate={required} />
I also did the same thing for field arrays. So when you were mapping through the array and needed to get the value off an index, it would have a type.
There was a problem hiding this comment.
That's very interesting.
I had no idea that you could do <Field<string> name="foo"> in TSX. That's wild.
There was a problem hiding this comment.
Yea, I've used it a couple of times. I think it's been in typescript since version 2.9 or so. Just allowing for a generic like that could allow something like the Form component to infer types sometimes too.
I'd be happy to try and implement the solution above after this PR would get merged. I think at least having generic typing for the Form component would be very nice instead of it just being set to object.
Codecov Report
@@ Coverage Diff @@
## master #516 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 15 16 +1
Lines 231 232 +1
Branches 56 56
=====================================
+ Hits 231 232 +1
Continue to review full report at Codecov.
|
|
Published in |
|
Hi @erikras, I think it would have been more appropriate to land this update in the next major version. Upgrading from 6.0.0 to 6.1.0 may break applications (which was my case), even if it doesn't affect any executable code. I'm super into it though. Better typings greatly improve developer experience, thanks for the update ! |
|
|
||
| let instance: React.Context<?FormApi<any>> | ||
|
|
||
| export default function getContext< |
There was a problem hiding this comment.
@erikras what's the reason behind this? kinda surprising, is this lazy evaluation needed for anything?
There was a problem hiding this comment.
It's not about lazy, it's about typing. I couldn't figure a way to handle the typing.
I suppose I could've just cast the thing...
|
@debel27 How did it break you? Could you prepare a repro case? Maybe the types could be tweaked to be backward-compatible. However, when dealing with types in JS I wouldn't assume that patch/minor can't break them. TS itself evolves and it's impractical to tie major versioning with types. |
|
I debated making this a major version bump or not. If it were "support for redux v7" or some popular external library, it would need to be a major version, but because FF and RFF are so tightly coupled, it didn't feel quite so "breaking". |
|
@Andarist, what broke for me :
Fixing those took me less than a minute (went from 17 Flow errors to none). I was just pointing this out in case you didn't think about it. But you did, so that's fine by me :) |
|
Thanks for pushing back. Good to have people providing such feedback. 👍 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Depends on final-form/final-form#223.