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

Unexpected fields values with useFieldArray when setting defaultValues #924

Closed
ryanwalters opened this issue Jan 28, 2020 · 18 comments
Closed
Labels
question Further information is requested

Comments

@ryanwalters
Copy link
Contributor

ryanwalters commented Jan 28, 2020

Describe the bug
I'm not 100% sure that this is a bug, but I know that what is happening is not what I expected.

When setting defaultValues on an array with useForm, I would expect the result fields array using useFieldArray to contain those values as-is rather than becoming mangled.

Example:

// In our case, the default values match a GraphQL schema so
// we can't necessarily change the shape

const { control } = useForm({
  defaultValues: {
    sources: ['abc.com', 'def.com']
  }
});
const { fields } = useFieldArray({ control, name: 'sources' });

console.log('fields', fields); 

// Expected output
/**
[{
  "value": "abc.com",
  "id": "5f0137bf-e201-425e-a7b0-f6cf76e0b3ca"
},
{
  "value": "def.com",
  "id": "5a93295c-7d9e-4762-a003-52406a611882"
}]
*/

// Actual output:
/**
[{
  "0": "a",
  "1": "b",
  "2": "c",
  "3": ".",
  "4": "c",
  "5": "o",
  "6": "m",
  "id": "5f0137bf-e201-425e-a7b0-f6cf76e0b3ca"
},
{
  "0": "d",
  "1": "e",
  "2": "f",
  "3": ".",
  "4": "c",
  "5": "o",
  "6": "m",
  "id": "5a93295c-7d9e-4762-a003-52406a611882"
}]
*/ 

This issue then snowballs into other issues when trying to display, append, delete, etc the values of the fields, but I figured I'd start here and step through the other issues only if needed.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/upbeat-bose-pt26m
  2. Look at console
  3. See issue described above

Codesandbox link
https://codesandbox.io/s/upbeat-bose-pt26m

Expected behavior
See Expected output in the code snippet above.

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Chrome
  • Version: 79.0.3945.130
@bluebill1049 bluebill1049 added the duplicated duplicated issue label Jan 28, 2020
@bluebill1049
Copy link
Member

I think a lot of people having this concern and question: #919

i will update the website soon, i think you are expecting the form to be controlled, but indeed it's not, watch() API is not same as state, watch is for watching input change after input rendered.

@ryanwalters
Copy link
Contributor Author

Ah ok, I only included watch to compare the values side by side. The values inside of fields are what I was trying to point out. I removed watch from the codesandbox example. It's possible I'm misunderstanding something, in which case I wouldn't mind seeing that updated documentation. 😄

@bluebill1049
Copy link
Member

I will do that tonight :)

@bluebill1049
Copy link
Member

I may have a work around, will try out tonight

@ryanwalters
Copy link
Contributor Author

Digging into this a little bit, it looks like the issue stems from appendId (see mapIds.ts#L12). That function spreads value, which doesn't work as intended with string values. This would seem to only work when fields is an array of objects, and give inconsistent results for other values (e.g. array of strings, array of arrays).

For consistency's sake, would it make more sense to keep the shape of fields the same regardless of the type of value? Removing the spread might be enough, assuming it doesn't cause any regressions.

Example:

// Before

export const appendId = <
  FormArrayValues extends {
    id?: string;
  } & FieldValues = FieldValues
>(
  value: FormArrayValues,
) => ({
  ...value, // <-- Change this guy
  id: generateId(),
});


// After

export const appendId = <
  FormArrayValues extends {
    id?: string;
  } & FieldValues = FieldValues
>(
  value: FormArrayValues,
) => ({
  value, // <-- To this
  id: generateId(),
});

@bluebill1049 bluebill1049 added this to In progress in React Hook Form Jan 29, 2020
@bluebill1049
Copy link
Member

bluebill1049 commented Jan 29, 2020

some good progress @ryanwalters try this CSB: https://codesandbox.io/s/bold-sound-7lej4

@bluebill1049 bluebill1049 added the status: working in progress working in progress label Jan 29, 2020
@bluebill1049 bluebill1049 changed the title Unexpected fields values with useFieldArray when defaultValue is set Unexpected fields values with useFieldArray when watch fields Jan 30, 2020
bluebill1049 added a commit that referenced this issue Jan 30, 2020
* #924 experience imporvement over watch with field array

* working in progress

* fix empty value situation

* improve type

* update unit tests

* adding unit test coverage

* include more unit tests for useFieldArray

* convert fill empty array into function

* change to more efficient array method

* improve unit test and fix errors and touched fields shuffle
@bluebill1049
Copy link
Member

react-hook-form@4.7.3-next.0 try this next version

@bluebill1049 bluebill1049 removed the status: working in progress working in progress label Jan 30, 2020
@ryanwalters
Copy link
Contributor Author

ryanwalters commented Jan 30, 2020

I'll give it a look at some point today. Thanks for the speedy turnaround!

Edit:
I updated my codesandbox to use react-hook-form@4.7.3-next.0 and the issue still exists.

I think there was a miscommunication somewhere. I see you updated the title of this issue, but my issue wasn't with watch, it was with the value that fields receives when setting defaultValues.

Please see the output of my codesandbox and my comment above with a possible fix. I can also create a PR if that's easier.

@bluebill1049
Copy link
Member

oh sure PR welcome @ryanwalters

@bluebill1049
Copy link
Member

Digging into this a little bit, it looks like the issue stems from appendId (see mapIds.ts#L12). That function spreads value, which doesn't work as intended with string values. This would seem to only work when fields is an array of objects, and give inconsistent results for other values (e.g. array of strings, array of arrays).

yes we only support objects

@bluebill1049
Copy link
Member

sorry for not reading your issue carefully... that was my bad. yes, right now we don't only support object due to the nature of uncontrolled inputs which required unique id during mapping. I will update documentation accordingly. apologize for not reading your issue carefully, I get overwhelmed by issue around watch() and assume the issue was the same.

@bluebill1049 bluebill1049 added question Further information is requested and removed enhancement New feature or request labels Jan 30, 2020
@ryanwalters
Copy link
Contributor Author

yes we only support objects

Are you open to supporting strings and arrays as well as objects? It seems like a useful feature to me (obviously 😄). We can probably work out an API that is not a breaking change.

The change I proposed above would be a breaking change, but seems cleaner to me. A non-breaking solution would be to set a value property only when the incoming value is not an object. I'm happy to open a PR for either solution, just let me know.

@bluebill1049
Copy link
Member

yes we only support objects

Are you open to supporting strings and arrays as well as objects? It seems like a useful feature to me (obviously 😄). We can probably work out an API that is not a breaking change.

The change I proposed above would be a breaking change, but seems cleaner to me. A non-breaking solution would be to set a value property only when the incoming value is not an object. I'm happy to open a PR for either solution, just let me know.

hey @ryanwalters I guess it's tradeoff with FieldArray uncontrolled, because it will need to be an object which contains a unique ID for the map, however if you have better suggestion or idea why not! :)

ryanwalters pushed a commit to ryanwalters/react-hook-form that referenced this issue Jan 31, 2020
…ues, assign other values (e.g. strings, arrays) to value property
@ryanwalters
Copy link
Contributor Author

Opened #940. The solution should be backwards compatible, and keeps the unique id.

@bluebill1049
Copy link
Member

thanks, i added some feedback take a look there :)

@ryanwalters ryanwalters changed the title Unexpected fields values with useFieldArray when watch fields Unexpected fields values with useFieldArray when setting defaultValues Jan 31, 2020
@bluebill1049 bluebill1049 moved this from In progress to Done in React Hook Form Feb 2, 2020
@blakecannell
Copy link

blakecannell commented Feb 3, 2020

Hey guys, I'm still seeing issues here (on 4.8.0), whereby the array doesn't stay intact when calling getValues. Each item of the array is being serialised and added to the root of the form state. The state of getValues also seems to be one render behind (add an item, it won't show in state until you take another action/render).

https://codesandbox.io/s/react-hook-form-usefieldarray-b91x7

Illustrated in this sandbox by stringifying getValues & fields above the form.

Sorry if this is an unrelated issue (or if I'm not using useFieldArray correctly), though I was directed over to this issue from the separate one I created (#935).

Thanks in advance for your continuoued efforts on this component, it's much appreciated.

@bluebill1049
Copy link
Member

hey @blakecannell i think you want to use watch() instead getValues().

@ElchinIsmayilov
Copy link

I'll give it a look at some point today. Thanks for the speedy turnaround!

Edit:
I updated my codesandbox to use react-hook-form@4.7.3-next.0 and the issue still exists.

I think there was a miscommunication somewhere. I see you updated the title of this issue, but my issue wasn't with watch, it was with the value that fields receives when setting defaultValues.

Please see the output of my codesandbox and my comment above with a possible fix. I can also create a PR if that's easier.

I'll give it a look at some point today. Thanks for the speedy turnaround!

Edit:
I updated my codesandbox to use react-hook-form@4.7.3-next.0 and the issue still exists.

I think there was a miscommunication somewhere. I see you updated the title of this issue, but my issue wasn't with watch, it was with the value that fields receives when setting defaultValues.

Please see the output of my codesandbox and my comment above with a possible fix. I can also create a PR if that's easier.

I am having the same issue, still no support for string. I kind of found a work around

{fields.map((field: any, i: number) => { const fieldValue = _.map(_.omit(field, ["id"])).join(""); return <input defaultValue={fieldValue} />; })

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
Development

No branches or pull requests

4 participants