-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
RFC: Formik 3 #2360
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
RFC: Formik 3 #2360
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/formik/formik/giol1txjc |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ea37493:
|
packages/formik/src/useFormik.tsx
Outdated
} | ||
|
||
const combinedErrors = deepmerge.all<FormikErrors<Values>>( | ||
[fieldErrors, formSchemaErrors, formValidateErrors], |
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 will throw an error in deepmerge.all
trying to convert undefined
to an object as formSchemaErrors
and formValidateErrors
don't always get set.
I know its still very WIP, but I've integrated this into one of our larger forms to try to address some performance issues and its a massive improvement! Going from >100ms for each update to <3ms |
const err = validate(eventOrValue.target.value); | ||
setError(err); | ||
} | ||
forceUpdate(); |
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 no longer any top-level state, is this call to forceUpdate
needed on each change?
I keep going back and forth on this as a concept. But I figure I'll write out my first impressions. On one hand, some state belongs at the field level; meanwhile on the other hand, I feel that this is solving a problem that React should solve on its own -- subscribing to a slice of context. The sum of the form's values, to me, is greater than the sum of its field's values. I believe the interactions between different fields, should be handled at a form level. Finally, this seems to me to be at war against the foundational tenets of Context-based functional React. I was super happy about the new v2 reducer pattern and was hoping v3 would come to embrace that more than anything. That said, it's such a major shift that it's possible I just haven't seen enough for it to click! |
In short, how dos this compare to what react-hook-form does? |
I'm curious how to handle cross field dependency when the state is now stored in each filed, so how can the dependent field component react to another filed component value change, since there is no global state which child siblings can receive as props or subscribe to their context to react to. |
While there is no global state, you can get access to sibling state by calling getValues() which loops through the registry and returns current values. Or better yet, we could even make a specific function called getFieldXXX() which could allow you to effectively just subscribe/get updates to a specific field’s state/props. |
Shortly after I wrote this RFC, React merged useMutableSource() which is effectively what I have reverse engineered here with forceUpdate AFAICT. |
Instead of this PR serving as an RFC, can we create an issue that is an actual RFC that lists all the requirements for Formik 3 and this can serve as an implementation of that RFC? That way we can discuss the merits of the RFC and the implementation separately. In fact, maybe we should create RFC for features and combine agreed upon features into Formik 3. We can formalize the RFC process in #1533
Features that I propose
Features from around the issues:
If we can formalize an RFC process, I'd gladly write a competing PR for Team Reducer (party of one). |
initialValues, | ||
validationSchema, | ||
}: UseFormikOptions<Values>) { | ||
const fieldRegistry = React.useRef<FieldRegistry>({}); |
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.
Why not just use a FieldRegistry
class here or createFieldRegistry()
factory instead of using a dozen useEventCallbacks()
? It can have a mutable .fields
property and this way all of its methods will always operate on the latest fields
. Wonder if I am missing something?
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.
fieldRegistry
instance has to be preserved between renders and scoped to only this instance of Formik
, hooks are currently the standard way to achieve that for a functional component.
It must be a ref in order to keep the values in sync across different hooks regardless of calling order.
This value will actually never change, so its inclusion in any hooks dependencies is just to satisfy eslint's exhaustive-deps
. Use of useEventCallback
is actually so that those callbacks don't cause re-renders in child components (it's like useRef, but for callbacks).
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.
Exactly, the idea is to have a bunch of never changing references. Can't we just assign it like:
const fieldRegistry = useRef(new FieldRegistry()).current
// Or even better, so as to not create a new registry on each render
const fieldRegistry = useMemo(() => new FieldRegistry(), [...])
The thinking is that all of the useEventCallback()
calls can be dropped altogether, since they will be defined as methods on the never changing registry instance. Furthermore if concurrent mode is considered to be an issue with this mutable source, this is just the right use case for the new useMutableSource()
hook that @jaredpalmer mentioned above.
This might be a little irrelevant, but I've always been fascinated by what JavaScript Proxy has to offer, if a pubsub model can be built around that, and let each field subscribe to that, and change in field can trigger rerender. It would be a nested tracked object that can react to changes made to that object. Are there are any specific disadvantages to this approach, I'm more interested in hearing your guys thoughts. |
@devarsh it isn't possible for Proxy to be polyfilled in the way we need it to be in IE 11 or earlier. Unfortunately, some demographics still use IE 11 due to enterprise restrictions etc, so I don't see support for it being dropped quite yet (it reaches EOL in 2025). Usually we can progressively enhance with polyfills, but with Proxy it's pretty much impossible due to it altering the behavior of javascript syntax itself at runtime. I used proxies in the following project to test strongly typing Fields and it worked really awesome. However, I was unable to get it working at all in IE11, so I had to abandon the project for now. |
It would be good abstract over validation schemas like in this solution https://github.com/react-hook-form/react-hook-form-resolvers |
@Voronar you should open up a separate issue with this feature request. I'll add it to the discussion we're having regarding the v3 API. |
So one thing I overlooked, or we need to address better is that this technique doesn't work well for radio inputs. The problem is that by registering by { name: element }, the last radio input wins the registration battle. This means that when we go to set the value in response to a change, the other sibling radio inputs don't get the update (because their refs are no longer in the registry). This problem technically existed w/field-level validation on radio inputs in every version of Formik, but it didn't matter because you could just put validation on the last one. Now that we track values in the registry, this is more problematic. As a workaround, you can just use use-field or literally any other this works.... const RadioGroupField = React.memo(({ label, id, name, choices, required, ...props }) => {
const [{ value, ...field }, meta] = useField({ name });
return (
<>
<div id={id}>
{label} {!required ? <small>(optional)</small> : null}
</div>
<div>
{choices &&
choices.length > 0 &&
choices.map((c: any) => (
<div key={c.id}>
<input
aria-describedby={`${id}-error`}
value={c.title}
checked={c.title === value}
{...field}
type="radio"
name={name}
id={c.id}
/>
<label htmlFor={c.id}>{c.title}</label>
</div>
))}
</div>
{!!meta.error && !!meta.touched ? (
<div id={`${id}-error`}>{meta.error}</div>
) : null}
</>
);
}); this will not work as expected because the last field is the only one in the registry so the others don't get updates (although it worked in Formik 2) <Field type="radio" name="color" value="blue" />
<Field type="radio" name="color" value="red" /> I think the solution is to alter the registry into |
Closing as this ref-based approach has been dropped for #2846 |
This is my very very early MVP of what could be Formik 3. It fixes some long standing issues related to performance by inverting Formik's core to be field-level instead of form-level.
It works by moving often-changing field state into refs (and local state) in each field, but then registering each field's refs on mount to a parent context. Fun fact: this is actually how we do field-level validation in Formik 1 and 2.
In order to "fake" top-level state, which no longer exists, we implement few new helpers:
getValues()
,getTouched()
, andgetErrors()
, which all walk the ref registry of (mounted*) fields and grab the latest value off each ref. We also fakesetValues
,setErrors
, andsetTouched
similarly, calling each field's respective registered updater fn. These top-level methods call aforceRender()
internally so that all the fields get the latest data.isSubmitting
(andfocus
,submitCount
, etc.) stay at the top level as all field's need to know about them. We can debate wherestatus
and anapiError
should go.* we should make a way for folks to keep field state around even when the component containing
useField()
unmounts.There are a lot of
@todos
in the codebase as I didn't finish implementing all methods. In addition, there are some other breaking changes:isValidating
since all validation is sync now.handleChange
orhandleBlur
field.onChange
andfield.onBlur
returned byuseField()
cannot be curried anymore. They are already scoped to the field.field.onChange
andfield.onBlur
returned byuseField()
can intelligently handle input events or values (instead of just checking strings).useField
'smeta
andhelpers
. It's all called justmeta
right now.Todos
In order to support nested values / dotpaths, we need
getValues
,getTouched
, andgetErrors
to now unwind flat state keys into a potentially nested object. So that'social.facebook' : value
becomes{ social: { facebook: value } }
in an efficient way.useFormik()
handleSubmit
handleReset
submitForm
resetForm
validateForm
validateField
enableReinitialize
validateOnMount
validateOnSubmit
validateOnChange
validateOnBlur
setStatus
status
setValues
setTouched
setErrors
setSubmitting
withFormik()
<Formik/>
(render prop)useField()
parse
format
formatOnBlur
validateOnChange
validateOnBlur
meta
setValue
setTouched
setError
isDirty
isPristine
isFocused
?formik
object to be passed to useField() as second argument so that people do not need to use context.<Field />
<Form />
<FieldArray />
useFieldArray()
Soft Deprecations (warnings)
getFieldProps
,getFieldMeta
(I think we can reverse engineer these?)Hard Deprecations
formik.handleChange
formik.handleBlur