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

Make fields of useFieldArray return an array of key-value pairs #8197

Merged
merged 7 commits into from
Apr 16, 2022
Merged

Make fields of useFieldArray return an array of key-value pairs #8197

merged 7 commits into from
Apr 16, 2022

Conversation

KurtGokhan
Copy link

I know this is a long shot, but I thought it would be more convincing if I opened a PR. This is the PR for my comment here

Basically, I think the fields array should return the data without a loss. Previously, we could change the keyName option to some key we know that does not exist in our original object. But in the v8, there won't be a keyName option and it will be always id, which conflicts with an existing property very often. There is a plan to change it to key, but that could also conflict for some people & projects.

I suggest we separate the key and value prop to clear this confusion once and for all. I made alternative suggestions in the discussion. I would also be willing to help in that direction if that is what's agreed upon.

This is the gist of the change, and the rest is changes to tests.

https://github.com/react-hook-form/react-hook-form/compare/react-18...KurtGokhan:v8-field-array-key-value-pair?expand=1#diff-ee762f75b15dde4fde77049f36e387fa0421f37dc1c29dad72fceba8a23c0e4dR25

The number of changes to tests gives an idea of how much of a breaking change this is. Typescript will catch on missing id property and will warn the users. However it could be problematic if people don't use Typescript and they are unaware of this change.

@Moshyfawn
Copy link
Member

You know, it actually makes a lot of sense.

Now, I haven't thought of all the use-cases yet, but I like it

@bluebill1049
Copy link
Member

Thanks for the feedback and address that in a PR. I am happy to change the key from 'id' to 'key' which was already something we already kind addressed in v7 because it overlaps with the user data field id. however, I am not too keen to introduce another level of { value, key } as it's already working with v8, which no longer needs to message with the keyName

https://codesandbox.io/s/sweet-thompson-cvg6od?file=/src/App.js

let me know if the above codesandbox works for your use-case, if so, feel free to adjust your PR to change id to key, we can mark another task closed for v8.

@bluebill1049
Copy link
Member

Basically keyName change is no longer requirement even in today's world (v7)

@@ -382,8 +382,8 @@ export function useFieldArray<
fields: React.useMemo(
() =>
fields.map((field, index) => ({
...field,
id: ids.current[index] || generateId(),
value: field,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to avoid this breaking change.

@KurtGokhan
Copy link
Author

Yes, it works when you bind it to an input with name. But there is no way to get the original id of the field inside fields.map. In your example that would be line 32. You may ask why I need to get the id, I could just bind it to an input. But in practice I needed to get id and do some custom logic with it. I have 3 completely distinct use cases where I needed it. I guess this kind of pattern emerges as the app gets more complex. I will try to demonstrate the use case with an example later.

@KurtGokhan
Copy link
Author

KurtGokhan commented Apr 16, 2022

By the way, I would be happy only with the id->key change. In relational databases, an id field exists in almost every object. So just making the key name key would avoid most of the conflicts. I am just trying to raise an awareness that getting contented with only this change could also cause the same problem for someone else. (But I guess they could use Controller in that case to get the original key prop)

@bluebill1049
Copy link
Member

By the way, I would be happy only with the id->key change. In relational databases, an id field exists in almost every object.

I am happy to proceed with the above, and the other use case you pointed out above, you can get that from getValues().

Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥇

@@ -30,7 +30,7 @@
"build:modern": "rollup -c ./scripts/rollup/rollup.config.js",
"build:esm": "rollup -c ./scripts/rollup/rollup.esm.config.js",
"prettier:fix": "prettier --config .prettierrc --write \"**/*.{ts,tsx,css}\"",
"lint": "eslint '**/*.{js,ts,tsx}'",
"lint": "eslint \"**/*.{js,ts,tsx}\"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old one was not working for me. Maybe it is specific to Windows?

@bluebill1049
Copy link
Member

image

@bluebill1049 bluebill1049 merged commit 2cd93ca into react-hook-form:react-18 Apr 16, 2022
@KurtGokhan KurtGokhan deleted the v8-field-array-key-value-pair branch April 25, 2022 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants