Extend setValue functionality

[samstarling]

Formik is great, but there are one or two parts where the methods could be a little more TypeScript friendly, and in this PR I'm introducing a new method which I hope takes a step in this direction.

For context: I have a form where a user can enter an ID into a field to perform a lookup. Data is fetched over HTTP, and I use data from the response to populate other form fields by using multiple setFieldValue calls. The signature of setFieldValue isn't type-safe – the value is any, and the field may not necessarily exist. (There was some discussion of this in #1388)

This PR introduces a setFieldValues method that takes a Partial<Values> and merges that with the existing form values. I would find this really useful.

Things that I'm not too sure about, and that I'd welcome feedback on:

• Does this method go against the design of Formik, where fields are referenced using the path-style string (eg. people.0.first_name)?
• Is there a better name for this method, than setFieldValues? It's basically like setValues, but it allows subsets and merges rather than overwriting.

[samstarling]

@jaredpalmer If you had any ideas on why yarn test and yarn build succeed locally, but not on Circle CI, they would be much appreciated!

[johnrom]

@samstarling it looks like the specific version of TypeScript that is used in the build requires an explicit FormikConfig<V extends object> in order to use { ...values }. I also had this issue over in #1334, and I fixed it by upgrading typescript to 3.3.x in 3e1d0d9

Is there a better name for this method, than setFieldValues

I'm a bit confused as to why we need a new function instead of modifying the behavior of setValues to accept a partial state, and an optional shouldValidate parameter. It seems to do the exact same thing as setValues. FormikState<Values>['values'] is the same thing as Values, but in the future there may be different functionality needed for the formikState values property, so I'd recommend keeping it written the other way. For example, a future version of Formik might not require initialState, and FormikState.values might be Values | undefined.

[samstarling]

@johnrom Thanks for pointing that out – really useful, and I'll copy across that 3.3.x upgrade. Regarding the difference with setValues is that it calls this.setState({ values }), which means that values is completely replaced with what's passed in (as far as I understand). The method added in this PR just performs a partial update of values.

[johnrom]

Oh duh! Hope the TS fix helps.

[samstarling]

@johnrom It worked: thanks!

[Andreyco]

How about this signature?

setValues(values: Values | (values: Values) => Values)

Basicaly, you pass setter funcion and you decide whether to merge or replace values. No breaking change! Cc @jaredpalmer

[jaredpalmer]

@Andreyco agreed this is a wart.

[jaredpalmer]

Oh I’m sorry. Why do we need this? We already have setValues?

[samstarling]

@jaredpalmer As far as I understand it, setValues replaces values entirely. @Andreyco: Do you mean that in the signature (values: Values) => Values, the property passed in would be the existing values? (Also, could you define wart – a bad thing, I assume?)

[johnrom]

I like the setValues(Values | ReplaceValuesCallback) API for back compat. If someone hackishly found a use case for passing a function as Values their code would break, but I'd assume that would have broken Formik itself long before finding a use case. I also think it's reasonable to want to skip validation during this call.

Edit: Also with @Andreyco 's method, no longer need to update typescript as all the object destructuring will happen in your project, and you can update typescript yourself.

[Andreyco]

I am against introducing new API for this.

To explain my previous comment with code...

// Now, Formik's setValues replaces values altogether. Merging is "hard", you need to pass all values
formik.setValues({ anotherField: 'value' }) // -> bad, original values are lost
formik.setValues({ ...allValues, anotherField: 'value' }) // -> good, but... passing values is required

// With setter callback, values could be passed by Formik
formik.setValues(values => ({ ...values, anotherField: 'value' })) // -> good, same as React's setState, values are passed. You decide whether to merge or replace

No new API, backward compatible.

The other thing is I cannot imagine situation when you would want to replace original fields with other ones (can we actually type this?!), thus merge would be what I expect, default behaviour. But this would be breaking change.

[samstarling]

@johnrom @Andreyco @jaredpalmer Thanks to you all for your feedback – I've just pushed a commit which implements your suggestion. Could you take a look and let me know what you think?

[johnrom]

With the last revisions, I'm liking this API. You should merge master into this branch so that there are no conflicts. If TypeScript has been updated in master, that will remove the need to update TypeScript.

Finally, it might be useful to make this callback async friendly in the future, similar to what was proposed in #909. But the process of using Promises for callbacks like this hasn't been established in v1 that I know of.

[samstarling]

@johnrom Thanks – I might leave the async-friendliness until later then, if the process hasn't been established yet. For the .size-snapshot.json, how should I resolve that conflict? This branch also appears up to date with master, at least to me!

[Andreyco]

Rebase, please

[samstarling]

@Andreyco I've just rebased

[Andreyco]

@samstarling would you be so kind and go thru change requests? TY!

[samstarling]

@Andreyco Absolutely - thanks for all the reviewing, I’ll address the feedback tomorrow!

[samstarling]

Hey everyone: the only outstanding change now is the documentation, but please let me know if I missed anything. I asked a question above about how the types are documented – once I know how to proceed, I'll update the docs and we should be good to go.

[jaredpalmer]

This PR should be against v2.

[samstarling]

@jaredpalmer Ah, I hadn't realised that, sorry – how can I fix that?

[PatoBeltran]

@jaredpalmer I just rebased from master and fixed the conflicts, anything else needed?

[johnrom]

Tiny nitpick; overall I think the code looks good (I haven't tested)

[jaredpalmer]

Suggested change

	#### `setValues: (valuesOrValuesFactory: ValuesOrValuesFactory<Values>) => void`
	#### `setValues: (valuesOrValuesUpdater: Values | (v: Values) => void) => void`

I think this is clearer.

[jaredpalmer]

Update: We could just use React.SetStateAction<S>

[jaredpalmer]

I can't push up my changes?

[jaredpalmer]

@samstarling can you give me the ability to edit this PR?

https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

[samstarling]

Hi @jaredpalmer – sorry for the delay, I was away on holiday. You should be able to push now.

[zetavg]

Hi everyone, is there any progress on this PR?

I think the ability to use updater functions as the setValues argument is necessary for some use cases that are setting the value based on the previous value, such as a counter that pressing a button will increase its value.
By using updater functions, possible race conditions can be avoided and that's an important reason why React the such called "Functional updates" functionality for setState.

I'm interested in working on this PR (or another if preferred) to provide such functionality.

[GunnarSturla]

This PR looks like it was really close, any chance of it being picked up again, or would it be better to start a new one?

[johnrom]

I had assumed this was closed because the changes were integrated into another branch. If that's not the case, we should reopen.

[GunnarSturla]

I can't find these changes on any other branch. It seems to have been closed shortly after being labelled stale.

I assume the PR needs to be rebased against a current branch (master or canary?) and, reading through the comments, I only saw some minor type fixes being suggested by @jaredpalmer; otherwise the PR could be good to go.

Is there anyone with edit permissions willing to take it over the finish line? @samstarling or @PatoBeltran maybe? I could also do it, if needed

Thanks for all your work, I think this PR will be a good addition to formik!

[relaxomatic]

I would also really like this, this would be so much better for us. Avoid stale values and prevent the need for passing around the values object when we only need to update a few fields. We could really simplify our code with this change.

What do we need to do? I can help?

[samotoo]

Wishing to see this PR get merged, it is neccessary to prevent stale values. At the moment I am using continuous setFieldValue calls to update multiple values, the enhanced API do help make the code more readable.

[PatoBeltran]

Hey @jaredpalmer I updated the branch for this PR with latest master and refactored to use React.SetStateAction as the type instead of creating a new one. Can we reopen this PR? It would be nice to see this in the next formik's release.

[PatoBeltran]

@jaredpalmer should I try to open a new PR to get traction on this functional updates with setValues feature or can we just reopen this one since the comments have already been addressed?