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

Ensure that validated values object is same object that is saved to state (+ Reduce number of actions) #2732

Closed
wants to merge 1 commit into from

Conversation

MaximSagan
Copy link

@MaximSagan MaximSagan commented Sep 7, 2020

The problem

The main problem I set out to solve with this PR was the creation of two different values objects on every setFieldValue call.
Namely, https://github.com/formium/formik/blob/c583eccc10378cbfc476c4cff3077e65b8e847ef/packages/formik/src/Formik.tsx#L79 and
https://github.com/formium/formik/blob/c583eccc10378cbfc476c4cff3077e65b8e847ef/packages/formik/src/Formik.tsx#L597
The former is used to update the state.values property, and the latter is used to validate the form (and ultimately update the state.errors object). The problem comes when the user wants to perform any kind of memoization of complex validation results, the values object that gets validated initially on setFieldValue is lost (a different object is saved to state) so it cannot be used for comparison to determine whether to re-run validation.

What I've done

To solve the problem stated above, on setFieldValue, I create a single new values object, and this object is used to both update state and in validation. With this change, the distinction between the SET_FIELD_VALUE action and the SET_VALUES action was not needed, so I removed the SET_FIELD_VALUE action.

For consistency, I also removed SET_FIELD_TOUCHED and SET_FIELD_ERROR actions.

@jaredpalmer
Copy link
Owner

@johnrom i think this is fine?

@johnrom
Copy link
Collaborator

johnrom commented Sep 15, 2020

I haven't tested, but based on a code review this looks like a nice performance enhancement. the only downside is that validations can now modify the values object. but so can any other part of the code, so I don't think it matters too much.

@MaximSagan
Copy link
Author

@johnrom

the only downside is that validations can now modify the values object. but so can any other part of the code, so I don't think it matters too much.

This is true, but it's worth noting that validation was already able to modify (mutate) any child-objects/arrays of values that were not directly created by the setIn() call.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Nov 20, 2020
@johnrom johnrom removed the stale label Nov 20, 2020
@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Dec 21, 2020
@github-actions github-actions bot closed this Feb 20, 2021
@johnrom johnrom removed the stale label Feb 21, 2021
@johnrom johnrom reopened this Feb 21, 2021
@vercel
Copy link

vercel bot commented Feb 21, 2021

@MaximSagan is attempting to deploy a commit to the Formium Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Mar 24, 2021
@johnrom johnrom removed the stale label Mar 24, 2021
@johnrom
Copy link
Collaborator

johnrom commented Mar 24, 2021

If I understand correctly, this isn't actually the same because the dispatch has the actual latest state as serialized by the reducer, whereas the state accessible to the setFieldValues and other functions could be stale, especially in the case of calling it multiple times per render.

This will be possible in #3089 because we provide getState() for exactly this purpose. @MaximSagan I think you should rebase this off of that branch and it can be merged once that is merged.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Apr 28, 2021
@github-actions github-actions bot closed this Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants