Closed danielkcz closed 5 years ago
I don't have a chance to confirm this theory works right now, but if setFieldValue and setFieldTouched are useCallback'd correctly, it should help to depend on those callbacks instead.
const { setFieldValue, setFieldTouched } = useFormikContext()
const setField = React.useCallback((field, value) => {
setFieldValue(field, value)
setFieldTouched(field, true)
}, [setFieldValue, setFieldTouched])
I am not against the idea of splitting formikContext into formik
and formikState
where formikState
is a snapshot and formik.currentState
would be the live state. I think this would be super helpful for async operations.
@johnrom Yes, you are right, that works, but I wouldn't call it the best solution. You can hardly explain to users that they have to destructure. Personally, I like keeping formik
to basically namespace variables. In a more complex code, it becomes vital to know where is which variable coming from.
Dang it, I was in the midst of an edit! I like the idea of keeping a stable Formik reference and snapshotting the state. I was responding with a solution because it was marked as bug report. But in terms of requesting a Feature, I'd +1. My .02 on an API:
const [formik, formikState] = useFormikContext();
where formik.state.current
is the "live state" useful for access from async operations a la this post
Yea, you are right that it's partially a feature request, but I think many users will come head banging about it being an actual bug. I would not blame them, it's not exactly an easy concept to comprehend.
It would be best to figure this out before V2 comes out for sure because it's going to be a breaking change.
I am also thinking if react-tracked
could help here ... #1676
I think I need to think thru this more
It shouldn't be actually that hard to achieve. Basically, we need to split the ctx
at this line...
As long as all API functions are wrapped into useEventCallback
, we just shove them into a persistent object like const [api] = React.useState(() => { ...fns })
. And then the tuple [state, api]
can be pushed to the context.
Additionally, there could be two hooks useFormikState
and useFormikApi
for easier consumption, but that's just tiny detail. Could be more approachable than mystical useFormikContext
.
I think I like the array destructuring better from both a documentation and quality of life point of view, but I have generally embraced the concept of TypeScript tuples so maybe that is really specific to the TS realm.
https://www.typescriptlang.org/docs/handbook/basic-types.html#tuple
Another question I have is whether or not formikApi
would update when enableReinitialize
is used and props change.
Finally, (and sorry for the spam!), I like const [state, api] = useFormikState()
because it matches nicely with const [state, update] = useState()
.
Another question I have is whether or not
formikApi
would update whenenableReinitialize
is used and props change.
The function references on api object can change, but they need to be stuck to a stable object that will never change.
It's kinda funny how hard is to do that. I am using MobX most of the time which builds around mutable and stable object references and it would be super easy to do there. And it would most likely be more performant. But I suppose that would be a completely different project in the end :)
Makes more sense like this to me...
const [state, api] = useFormikContext()
// these two are optional and can be done in userland
const state = useFormikState()
const api = useFormikApi()
I disagree and I think there should be a decision between whether API is immutable but changes over time (new object), or all of the functions of the API are designed to never change similar to the dispatch
from useReducer
. If a user wants a mutable reference they can call const apiRef = useRef(api)
and use apiRef.current
.
To add an example given your original post, imagine what happens if formik.setFieldValue
is updated without a new formik
object.
const formik = useFormikContext()
const setField = React.useCallback(field, value => {
formik.setFieldValue(field, value)
formik.setFieldTouched(field, true)
}, [formik])
@johnrom I don't think it would be a problem in that case. Since the formik
has a stable reference, anything you call through that will be always up to date. There is no closure problem happening.
or all of the functions of the API are designed to never change similar to the
dispatch
fromuseReducer
That's a bad comparison. The dispatch does not rely on a closure state. If you have a function that needs to access a closure variable then anytime that closure variable changes, you need to "refresh" that function so it has access to latest closure. That's why useCallback
needs deps/inputs so it knows when to refresh itself.
Correct me if Iām wrong, but in v1, all fns were like dispatch because they were bound as instance variables on the class. We can use useEventCallback to accomplish this with hooks IIRC.
@jaredpalmer Those fns are not the problem here, the ctx
object is. I've outlined a solution in https://github.com/jaredpalmer/formik/issues/1677#issuecomment-511295715. I don't think there is a way without splitting api & state, because you need both, the stability and mutability.
I'm looking through the code now, which I haven't had a chance to before, and I found that setFieldValue is a bad example because it uses useEventCallback
which I believe means it always returns a reference to the current function (as if you were using (setFieldValue = useRef(formik)).current.setFieldValue)
. However, there are things like handleChange, which is dependent on state.values
in order to determine whether or not a checkbox is currently checked, which would have to be mutated in order to properly update a checkbox in the current API (if I'm reading correctly). This means a user might do something like this:
const [state, api] = useFormikContext();
const { handleChange } = api;
api.setFieldValue("checkbox", true); // handleChange is now stale
const onCheckboxChange = React.useCallback(event => { handleChange(event.target.name) }, [api]);
return <input value={state.values.checkbox} name="checkbox" onChange={onCheckboxChange} />;
One might consider this incorrect usage, but with immutability or by converting handleChange to use useEventCallback
, we can avoid this issue altogether and prevent tons of github issues from popping up in the future.
Hmm.. we're saying the same thing. I somehow missed this:
As long as all API functions are wrapped into useEventCallback
And focused on this:
The function references on api object can change, but they need to be stuck to a stable object that will never change.
The function references wouldn't change, but the wrapped functions would. So we're arguing the same point. š¤£
Right, it needs to be carefully examined which function would go where. The mentioned handleChange
could be considered a state since it would be rarely called manually.
However, another way could be to basically keep the state object with the useRef
, update it whenever state changes (with useEffect). Any callback can always access the current
value of ref without the dependency on a closure. That would keep those functions static most of the time as well.
Checkbox is a bad example because they are now handled by Field in v2.
@jaredpalmer That was really unnecessary nitpicky comment :) Please let's focus on important discussion.
Btw, another idea is to actually split it into multiple contexts instead of a single one. That means each context can be updated in its own pace (or never). Check out the Informed library (kudos to @joepuzzo) for the inspiration.
https://github.com/joepuzzo/informed/blob/3c4c3018f3985e85f9e1b23865a2326e0cb871bc/src/Context.js
Notice also the getValues
present on the API object. That's also a great way of getting hands on the latest state without the need to subscribe to changes of the state.
@jaredpalmer I see checkbox handling in executeChange
on current master. Is that change in another branch?
These are the unwrapped, dependent handlers I could find on master.
resetForm
getFieldMeta
getFieldProps
handleChange
I think as opposed to using a getValues
or other type of API, we should expose things in a useRef-like manner (.current
) so the API conforms to React expectations. As I see it there are a few categories of objects being returned from useFormikContext, so we should come up with an API that factors them all in. Ideally, these would pair up nicely with TS types.
useReducer()
useRef && useEffect(() => ref.current = state)
initial-values, -errors, -touched, -status; validateOn*
useRef && useEffect(() => state.config.current = config
handle-blur, -change, -reset, -submit
isValid, isDirty
, Initial proposal:
const [state, api] = useFormikContext();
// where
state = {
...useReducer()
isValid,
isDirty,
config: formikConfig (initial*, validateOn*)
}
api = {
state: stateRef, (need to access via api.state.current so it's clear this could be "in the future")
config: stateRef,
...handlers,
...API,
}
Open questions:
getCurrentState()
, getCurrentConfig()
preferred? It would be weird of someone to overwrite those refs but it could be used as a hackSo it looks like this wasn't addressed in V2 at all. That's a shame, I was hoping to learn something new here. Oh well, it's unusable for me like this and I am expecting it is going to bite more people with Hooks adoption growing.
@FredyC in my opinion one should always use the most exact dependencies for these React fns, to prevent extra re-renders regardless of the implementation. I think this is still a valid feature request, but even if it was implemented I'd recommend people use [formik.setFieldValue, formik.setFieldTouched]
as they will always be the most accurate dependencies, and anything else (like [formik]
) would be subject to the whims of a changing API.
(The maintainers did not close this issue.)
Yea, I did close it because I could not afford to wait for this and other fixes and had to write my own solution from the ground up based on MobX which is not only faster (only fields, not a whole form are re-rendered) but does not suffer from these issues because of stable references for anything.
I'd recommend people use
[formik.setFieldValue, formik.setFieldTouched]
as they will always be the most accurate dependencies
It feels more like a workaround. Try to explain it to that ESLint rule which just sticks "formik" there when the auto-fix is enabled. Sure, you as a developer are responsible to handle deps correctly, but there is no clue for the user this is needed until an actual issue starts happening. And those can be pretty sneaky.
š Bug report / Feature request
Today, I cried...
Current Behavior
Consider the following contrived example. The ESLint rule
react/exhaustive-deps
immediately complains thatformik
needs to be in deps. Makes sense, why not? Ouch! Such callback will be recreated on every little change becauseuseFormikContext
always returns a new object.The callback alone wouldn't be too bad. But then comes
useEffect
into play and things get very bad. The ESLint will once again suggest adding callbacksetField
to deps. Originally the effect that is supposed to be executed once (on mount) is now executed on every little change.Lines of thinking here are like: "I have it wrapped in the callback, it's safe to have effect depend on it, right? Damn..."
Expected behavior
I am aware I can disable ESLint for that particular line and avoid adding that dependency there. That's not the point. Anyone can get burned by it because it's a good assumption it will work just fine.
Reproducible example
https://codesandbox.io/s/happy-dubinsky-91lhc?eslint=1&expanddevtools=1&module=%2Fsrc%2FFormMemory.jsx
Notice it grows geometrically for some reason and might kill your browser tab. But there is a button to actually start it, so don't worry to open and check the code first.
Suggested solution(s)
This is really tricky. One side of a problem is that recreating a new object will allow Context to re-render interested parties. With stable reference, no render would happen.
So I am thinking something along the lines of splitting state and api object and providing two hooks for accessing each. The api object can have a stable reference while state can still cause necessary re-render. And ESLint suggestions won't be a problem anymore.