Skip to content
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

[v2]: Distinguish between high/low priority validation #1522

Merged
merged 9 commits into from
May 29, 2019

Conversation

jaredpalmer
Copy link
Owner

@jaredpalmer jaredpalmer commented May 18, 2019

After thinking more deeply about useEffect in v2, I don't think useEffect() is suitable for modeling Formik's validation strategies. In the current version of v2, Formik is over-validating and there is no way to avoid validating on change because the "validateOnBlur"-effect depends on state.values. This dependency got me thinking that useEffect isn't the correct approach (furthermore, this was going to be a breaking change too).

This PR refactors validation so that it is overridable just like Formik 1.x. Furthermore, validation triggered as a side effect of an update (i.e. handleChange, setFieldValue, setValues, handleBlur, setFieldTouched, setTouched) is now wrapped in scheduler's unstable_runWithPriority. This tells React that the resulting update to state.errors is low-priority (or non-user-blocking). AFAICT, this is correct model of validation for onChange/onBlur. Conversely, validation triggered imperatively or during pre-submit via validateForm, validateField, submitForm, handleSubmit is the same as it was (high-pri/user-blocking).

In a different PR, I want to play around with removing async validation entirely from Formik. The thinking is the same as rationale as to why you should not use something like Redux for making async requests, but instead just use it a store.

Anyways, open to ideas. I also now need to fixup the tests to flush out some stuff.

@jaredpalmer jaredpalmer changed the title Distinguish between high/low priority validation [v2]: Distinguish between high/low priority validation May 18, 2019
src/Formik.tsx Show resolved Hide resolved
src/Formik.tsx Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Show resolved Hide resolved
@Andreyco
Copy link
Collaborator

Andreyco commented May 18, 2019

as much as I love hooks, I hope they won't introduce unpredictable inconsistencies/race conditions 🤣

src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
const setTouched = React.useCallback(
(touched: FormikTouched<Values>) => {
dispatch({ type: 'SET_TOUCHED', payload: touched });
return validateOnBlur

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this logic be part of handleBlur? I mean it feels strange to assume that setting touched is always because of the blur event. Same for setFieldTouched. If anything, it should be an optional argument here if revalidation is desired.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it works now in v1. tbh, validateOnBlur really means validateOnTouched the more i think about it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as I said, it would be nice to give the option to opt-out of this behavior for a setTouched call. In case I really do want to set touched for one field and avoid validation for some reason. I don't have a real use case though, so perhaps it's not worth it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. If we didn't need to keep v2 backwards compat, i would not have this side-effect stuff. This is something we will fix for v3.

src/Formik.tsx Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
@jaredpalmer
Copy link
Owner Author

Going to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants