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
π useFieldArray #768
π useFieldArray #768
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cc58895:
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7acf6f1:
|
Will add units and automation later as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far so good π
append, | ||
remove, | ||
update, | ||
insert, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be named as insertAt
because insert
is more like a generic term for insertion of values. Same goes with update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think insert is fine, less text to type as well. we have TS support the type as the argument :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Sounds good to me :) Thanks
src/useFieldArray.ts
Outdated
import * as React from 'react'; | ||
import isUndefined from './utils/isUndefined'; | ||
|
||
const id = () => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also name this generateId
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
src/useFieldArray.ts
Outdated
}; | ||
|
||
const insert = (index: number, value: any) => { | ||
setField([...fields.slice(0, index), value, ...fields.slice(index)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better than using splice
?
const newFields = [...fields];
newFields.splice(index, 0, value);
setField(newFields);
or is it about fewer lines the better or referential integrity of the values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to avoid splice
cause it's a mutation, slice
is always return new Array
setField( | ||
isUndefined(index) | ||
? [] | ||
: [...fields.slice(0, index), ...fields.slice(index + 1)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
splice
?
Sorry for pushing the |
all good :) i try to use slice if i can most of the time |
I thought it was better and all. Thanks for answering it up. :) |
thank you for taking a review on it @JeromeDeLeon β€οΈ |
You're welcome :) |
Looks like we need to bump it up to 6.6kb. π |
dame it π€§ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late!
This looks good!
thanks for taking a look @kotarella1110 |
Would you like to be able to set |
what do you mean @kotarella1110 ? are you refer to |
oops looks like it's not working @kotarella1110 good pick up!! |
@kotarella1110 that should make it work for insert. I am working over |
hmmm i don't think we need |
read my comment above, i have updated the comment. |
oooooh π just seen that if you hard reload my sandbox (I've added more test items to the array), the output from line 13 on DemoField.js renders a nested output (should it?), when you change the first array item (App.js - eg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Just a minor clarifications and all π . Sorry, I haven't able to support this one until today. Thanks for those who tested it out. We avoided bugs before the final release. π
@@ -0,0 +1,2 @@ | |||
export default (name: string, searchName: string) => | |||
name.startsWith(`${searchName}[`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is searchName
not enough? I don't think anyone will use the name
more than once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test
testmyname
safer to make sure it's an array :)
src/useFieldArray.ts
Outdated
import isArray from './utils/isArray'; | ||
import { FieldValues, UseFieldArrayProps, WithFieldId } from './types'; | ||
|
||
export const appendId = < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well move this to logic
folder if that's good to you just for consistency π .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do :)
|
||
const swap = (indexA: number, indexB: number) => { | ||
resetFields(); | ||
[fields[indexA], fields[indexB]] = [fields[indexB], fields[indexA]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π for using destructuring to swap values
@@ -985,6 +994,12 @@ export function useForm<FormValues extends FieldValues = FieldValues>({ | |||
defaultValuesRef.current = values; | |||
} | |||
|
|||
Object.values(resetFieldArrayFunctionRef.current).forEach( | |||
resetFieldArray => | |||
isFunction(resetFieldArray) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better than doing if-then
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
save few bytes here hehehe
Thanks @bluebill1049 my bad π
I think this might be the cause of my undefined issue as the shape changes. |
thanks a lot for the testing @simmo β€οΈI will address @JeromeDeLeon's comments tmr morning. almost 12pm here now going to bed. catch you guys tmr. thank you all π |
Looks good to me. It took me a while to understand that the indices in the fields map function are unique even if two of the indices share the same name. I am about to upgrade my project to use 4.5.0-beta.9. I'll report any issues here if I see any. |
I can't say for certain but I think useFieldArray is mutating my defaultValues. I'll try to post a codepen to see if I can replicate the behavior. I don't think seeing this earlier in beta.6. Also, when I pass in the defaultValues into useForm without useFieldArray I don't see the mutation. Yeah it looks like something is very broken. When I pass an object like this to defaultValues in userForm:
When I getValues from that I get back:
My envRoles are completely gone. That wasn't happening before. |
@thoughtassassin Sounds like you're having the same issue as me. Are you on beta 11? |
can you try the latest beta? |
@bluebill1049 is the latest beta.11? |
yes quite few iterations over issues |
@bluebill1049 working on it now. I think there may be an issue in my code. I am trying to isolate and identify it now. What I am seeing is that I have a few inputs in one component and I have my field array elements in another. The component without the field array gets the components but then they disappear from the field array. However, when I comment out the component without the field array elements, the field array elements show just fine. In the component that I have the other elements I have a wrapper around that component. It uses useFormContext. I think when that element is being initialized it is wiping out everything else. I'll see if I can create a codesandbox to see if I can duplicate the behavior outside of my project. That having been said, I don't think that I am doing anything unusual. I wasn't seeing this on Friday with version 6 but I think I may have started seeing before I even switched to beta.9. I am currently on beta.11 and seeing this behavior. |
Thanks for the update. See if u can reproduce in a code sandbox. I will take a look at it. |
are you using the same |
@bluebill1049 Okay. So I am not crazy. I can reproduce this issue without any of the other components in my codebase. If it looks like I am doing something wrong please tell me: https://codesandbox.io/s/react-hook-form-usefieldarray-8f2nd You'll notice that if you delete the NonArrayFields, ArrayFields shows. But if NonArrayFields is there ArrayFields are not rendered. This codepen is reproducing the issue that I am seeing exactly. I don't think I was seeing this Friday. |
Thanks will take a look at it :) |
beta10 is bad :( https://codesandbox.io/s/react-hook-form-usefieldarray-udu0s |
@bluebill1049 That fixed it! Thank you. |
Cool! Once I fixed code reviewβs feedback, we are ready then |
@kotarella1110 let me know if you still have any more review comments. aim to merge this tonight. |
Thanks everyone :) merged |
New custom hook for FieldArray:
β¨
useFieldArray
https://codesandbox.io/s/react-hook-form-usefieldarray-vy8fv
latest beta version:
4.5.0-beta.2