react-hook-form / react-hook-form

📋 React Hooks for form state management and validation (Web + React Native)
https://react-hook-form.com
MIT License
41.2k stars 2.06k forks source link

Feature request - better virtual support for field arrays #4385

Closed wdfinch closed 3 years ago

wdfinch commented 3 years ago

Is your feature request related to a problem? Please describe. Yes. At the moment there are two major issues with field arrays.

  1. Operations made on field arrays will only update the fields property and not form values. This means our form values are separate (fields and formValues) and not kept in sync. This leads to multiple issues and annoying workarounds.

  2. Field array elements must be connected (mounted) in order to mutate them. So in other words, if we want to change the value of a field array element, we must make sure that we do this while every element of the field array is mounted. Otherwise, non-connected elements can be removed, not updated right, etc. This means if we have a complex field array and only want to show a subset of it, we must use display: none or something similar. This leads to worse performance and more complex code as we might have other hooks and code run when a field array element is mounted (ex. a wizard form represented as a field array element).

Describe the solution you'd like I would like to see two solutions here.

  1. I don't think storing form values in separate places is a good idea. It makes our code harder to understand and leads to render cycles where formValues do not contain the correct values. I propose that formValues become the single source of truth regarding mutations. So for example, if we mutate a field array, then only formValues will be mutated and fields will simply be a snapshot of the formValues at a particular moment in time. This way we can always trust that formValues has the most current values.

  2. I would like to see better support for mutating field arrays that aren't mounted. First off, I think field arrays should stay connected once they are mounted for the first time. I'm not sure if this is how it works now, but it's more in sync with how regular inputs work in V7 forward (inputs are not automatically unregistered on unmount).

From here I see two possible solutions here:

  1. We are able to connect field arrays without having to mount them. So something like register.

  2. If we operate on a field array element that hasn't been connected yet, we simply modify the default values for that element rather than remove the current default values.

I personally like 2 more since it would be annoying to have to register/connect all our field array elements.

Describe alternatives you've considered I currently keep every field array element mounted and make sure I only use methods like setValue while they are mounted.

Additional context I think solution 2 would be helpful for all types of mutations beyond just field arrays. As a user, I would like to just run setValues and know for certain that formValues will be the object I pass in. I feel like every time I use this method, I don't know what's going to happen and it very much depends on what is mounted or not.

I get not wanted to allow arbitrary values into formValues. This is where default values can help. If we use setValue and we pass in some non-registered/connected value, perhaps we could modify default values in some way to still represent that value. I get modifying default values causes some issues with watch and useWatch, but perhaps we could find a solution here. Otherwise, I feel like I'm rolling the dice a lot of times on what setValue will actually produce when I have a complex form with conditionally mounted inputs.

Lastly, the library is designed around mounted inputs, using keys, and other view/dom-related tools to connect/register inputs. While this works fine for simple forms where everything is mounted at once, it makes working with more complex forms where we need to use conditional rendering much more difficult. It also seems like an anti-pattern as React is all about not using the DOM to hold state and control the application. This is a bit beyond this feature request, but it would be nice if there were options or methods we could use to move away from this reliance on mounting elements in order to ensure the correct operation of the library.

bluebill1049 commented 3 years ago

This is duplicated: https://github.com/react-hook-form/react-hook-form/issues/4359

wdfinch commented 3 years ago

There were a few additional ideas here such as the single source of truth for form values and virtualization support. I'd say this is related, but a bigger change or more extreme solution to the problem. We can leave it closed until the other get's some traction though.

bluebill1049 commented 3 years ago

Thanks @wdfinch but the root cause it's the same and that's why i marked it as duplicated. maybe able to get something work before we release the V7.

wdfinch commented 3 years ago

No rush. I do have a workaround using display none and just checking both formValues and fields to get the true state of my formValues. This would be more of a long-term change if people want it.

bluebill1049 commented 3 years ago

fields object is probably never going to be updated on each onChange, because it's going against the whole point of this custom field array (uncontrolled), my aim right now it getting setValue more flexible with field array, so at least that extra re-render is not required to reflect the update.

wdfinch commented 3 years ago

The uncontrolled fields are not a huge issue. It would just be an easier syntax to use as we wouldn't need to rely on getValues to ensure we are getting the most up-to-date value for default value assuming we are mounting and unmounting field arrays. I would totally keep the default uncontrolled version. My only suggestion was to maybe add an option to make it controlled unless this causes issues other than performance.

I'd say the biggest pain point is setValue having somewhat undefined behavior when it comes to changing the value of a field array unless we are calling it when the field array is mounted. I have a number of use cases where I need to call setValue and it's not really feasible to have the field array mounted or just impossible given some of the libraries I'm using (like forms in modals where the library unmounts the children when the modal is closed).

bluebill1049 commented 3 years ago

The uncontrolled fields are not a huge issue. It would just be an easier syntax to use as we wouldn't need to rely on getValues to ensure we are getting the most up-to-date value for default value assuming we are mounting and unmounting field arrays. I would totally keep the default uncontrolled version. My only suggestion was to maybe add an option to make it controlled unless this causes issues other than performance.

should be useWatch instead of geValues()

I'd say the biggest pain point is setValue having somewhat undefined behavior when it comes to changing the value of a field array unless we are calling it when the field array is mounted. I have a number of use cases where I need to call setValue and it's not really feasible to have the field array mounted or just impossible given some of the libraries I'm using (like forms in modals where the library unmounts the children when the modal is closed).

This is what i try to solve with setValue with unmounted field array.

wdfinch commented 3 years ago
  1. My bad. Either way, though it adds a lot of syntax. For example, if we have a nested array, it becomes watch().firstArray[firstArrayIndex].secondArray[secondArrayIndex].fieldWeWant. Would be much nicer if we could just write item.fieldWeWant and trust it has the most up-to-date value even if we take a performance hit. Hence it being optional.

  2. Gotcha. Do you mean it's solved now or it's still being worked on?

bluebill1049 commented 3 years ago

My bad. Either way, though it adds a lot of syntax. For example, if we have a nested array, it becomes watch().firstArray[firstArrayIndex].secondArray[secondArrayIndex].fieldWeWant. Would be much nicer if we could just write item.fieldWeWant and trust it has the most up-to-date value even if we take a performance hit. Hence it being optional.

uesWatch you can target that field array

Gotcha. Do you mean it's solved now or it's still being worked on?

working on.

wdfinch commented 3 years ago

Gotcha. Thanks for the update. After thinking about this a bit more, I think I have a solution to the first issue here.

The main issue is fields are not being synced with formValues on remount. I've been able to use getValues because I really don't need fields to be controlled. I really just need the most up-to-date value when the field array is remounted and after operations like setValue occur which already updates the fields correctly. What are your thoughts on syncing fields when the component using the related useFieldArray hook is remounted?

This would solve my issue as fields would be in sync with formValues, I could use the easy syntax, and we could keep the current design/avoid having to make fields controlled.

bluebill1049 commented 3 years ago

This would solve my issue as fields would be in sync with formValues, I could use the easy syntax, and we could keep the current design/avoid having to make fields controlled.

don't focus on fields, focus on what's return by useWatch and watch because they r what's subscribed to the input. fields only return the first render which needed for the defaultValue, and prefers useWatch over watch when you have a large list of items.

wdfinch commented 3 years ago

Sorry, I don't think I explained this the best. I'm specifically talking about initializing defaultValue regarding this issue. If we need to observe a field array value, then watch is totally the right way to go.

The issue is if I have a simple field array with no need for watch and I remount the field array after it's been mounted and user changed the inputs, then fields will not be up to date with what the user last entered and therefore defaultValue will not be the most recent value. Therefore I do something like defaultValue={getValues().arrayName.index.fieldName ?? field.fieldName} every single time I have a field array.

So in other words, if fields could be synced on mount then I could just write defaultValue={field.fieldName}. Using watch would be overkill for this.

bluebill1049 commented 3 years ago

The issue is if I have a simple field array with no need for watch and I remount the field array after it's been mounted and user changed the inputs, then fields will not be up to date with what the user last entered and therefore defaultValue will not be the most recent value. Therefore I do something like defaultValue={getValues().arrayName.index.fieldName ?? field.fieldName} every single time I have a field array.

i remember there is an issue with this, which i have resolved?

wdfinch commented 3 years ago

I don't think so. I've attached a sandbox to show what I'm referring to. If you type something into the field array and click the toggle button twice to unmount and remount the component, then you'll notice that the field is populated with the original default value rather than what the user entered. This is where getValues can help to make sure we use the most recent value as the default value on remount. So if fields could essential refresh or get the newest values if remounted, then this would solve my issues of having to also use getValues every time I want to initialize the defaultValue.

https://codesandbox.io/s/react-hook-form-usefieldarray-template-forked-92x1y

bluebill1049 commented 3 years ago

you can create a separate component and useWatch to watch its value, so it's always going to be updated one.

wdfinch commented 3 years ago

Yes so I know we can use useWatch to get the most update to date value. I guess I'm saying that for simple use cases where I just want to initialize the default value of a field array, this seems like overkill.

In other words, I don't really want the performance hit of useWatch and extra syntax. fields works fine for the default value except in the case of remounting a field array. If fields could be synced with form values when remounted, then I could just use fields to get the default value in most cases. There will still be cases where useWatch is appropriate, but for many simple field arrays, the fields property would be sufficient if it was resynced on mount or when reconnected.

bluebill1049 commented 3 years ago

yea, I hear you, it go back to the circle to make fields controlled because when the input gets unmounted, useFieldArray will not aware of that. I will seek a potential solution for this, but as of right now getValues() is probably the easiest solution.

wdfinch commented 3 years ago

Ok sounds good. To me controlled means update on every single input change rather than just on mount so perhaps there was some confusion here.

fields are already updated when setValue, append, and other modification methods are used. I guess I was thinking we would just add remounting to the list but as you mentioned it's probably not that simple.

bluebill1049 commented 3 years ago

fields are already updated when setValue, append, and other modification methods are used. I guess I was thinking we would just add remounting to the list but as you mentioned it's probably not that simple.

give me some time, I will investigate this to see if we have any option without sacrifice too much perf from hook form (perf means a lot to me, and probably one of the main reasons I started this project).

wdfinch commented 3 years ago

Sounds good. Thank you!

bluebill1049 commented 3 years ago

have a try on this one: https://codesandbox.io/s/react-hook-form-usefieldarray-template-forked-46onk?file=/src/index.js

wdfinch commented 3 years ago

Is this for using setValue when a field array is unmounted?

bluebill1049 commented 3 years ago

Is this for using setValue when a field array is unmounted?

I think this CSB may solve most of your requests.

wdfinch commented 3 years ago

So if the field array is mounted and then unmounted, then it seems like setValue works as expected. If the field array is not mounted and we use setValue, it still seems like we get undefined behavior.

Having the first item helps a lot though so thank you.

https://codesandbox.io/s/react-hook-form-usefieldarray-template-forked-yyhne

bluebill1049 commented 3 years ago

https://codesandbox.io/s/react-hook-form-usefieldarray-template-forked-46onk?file=/src/index.js https://codesandbox.io/s/react-hook-form-usefieldarray-template-forked-lp88s?file=/src/index.js

sorry to delete messages... as i found issue alone the way, looks better now.

wdfinch commented 3 years ago

This looks good. It seems like you achieved this in a beta version that is already out. So is this something that's achievable now?

bluebill1049 commented 3 years ago

This looks good. It seems like you achieved this in a beta version that is already out. So is this something that's achievable now?

No, they r all local src folders. I will release another RC this weekend. If this works we can close some of the feature requests as well.

Mendoza920 commented 3 years ago

This is duplicated: #4359

no

wdfinch commented 3 years ago

Ok thanks @bluebill1049.