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

Array prototype breaks react-hook-form validation #9037

Closed
1 task done
marcselman opened this issue Sep 14, 2022 · 6 comments · Fixed by #10328
Closed
1 task done

Array prototype breaks react-hook-form validation #9037

marcselman opened this issue Sep 14, 2022 · 6 comments · Fixed by #10328

Comments

@marcselman
Copy link

Version Number

7.35.0

Codesandbox/Expo snack

https://codesandbox.io/s/rhf-validation-issue-2cyqbk?file=/src/App.js

Steps to reproduce

  1. Add any method to Array.prototype from anywhere in code
  2. Create a form with useFieldArray with a required field
  3. Submit the form with the required field empty -> this will give a validation error
  4. Enter some data in the required field
  5. Submit the form again -> this will now give a validation error with an empty array as error

Expected behaviour

When submitting the form with the required field filled no validation error should occur.
Extra methods on Array.prototype (not overrides) should not affect react-hook-form

What browsers are you seeing the problem on?

Firefox, Chrome, Edge

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@marcselman
Copy link
Author

The problem does not occur if I use Object.defineProperty to add a method to Array.prototype.

if (!Array.prototype.myMethod) {
  Object.defineProperty(Array.prototype, 'myMethod', {
    value: function () {
      return true;
    },
    writable: false
  });
}

@marcselman
Copy link
Author

I've added another side-effect to the sandbox: the isDirty no longer gets updated as well.

@leapful
Copy link
Contributor

leapful commented Sep 14, 2022

@marcselman

Thanks for reporting the issue. The root cause is from util "cloneObject" which shallow copy object when there is a function inside object properties.

copy = isArray ? [] : {};
for (const key in data) {
if (isFunction(data[key])) {
copy = data;
break;
}

In this case, array has new prototype method "myMethod" therefore the whole array is kept as a shallow copy instead of deep clone and lead to share same reference between "defaultValues" and "formValues" which causes wrong form state checking (invalid , dirty).

@bluebill1049 : Can we add a double check "!Array.isArray(data)" before shallow copy "data" at line 19 ?

May be adding "hasOwnProperty" when using "for ... in ..." following javascript best practice to avoid any potential issue related to "prototype" is considered too.

NOTE: Using "Object.defineProperty" will bypass the issue because default setting "enumerable" is false therefore custom method "myMethod" will not show on "for ... in ..." loop.

@bluebill1049
Copy link
Member

Thanks for the investigation @leapful This is something we try to avoid with cloneObject in general, such as complex object type as moment or luxon. unless this is a generic use case around the community, we do not consider supporting it for now. It's a good practice to keep defaultValues as a pure value object instead of carrying methods/prototypes.

@bluebill1049 bluebill1049 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 14, 2022
@marcselman
Copy link
Author

@bluebill1049 Just to make sure you are aware:
If anyone uses the react-hook-form library in a project where somewhere in the project a prototype method or property is added to Array then react-hook-form will no longer function. And it's pretty hard to find or understand why so people might think your library doesn't work well (for them).
If that's fine with you, then maybe you could ad least add a check to show a console.warn if the Array object has been extended so people are aware and can fix it?

@John0x
Copy link

John0x commented Sep 18, 2022

Thanks for the investigation @leapful This is something we try to avoid with cloneObject in general, such as complex object type as moment or luxon. unless this is a generic use case around the community, we do not consider supporting it for now. It's a good practice to keep defaultValues as a pure value object instead of carrying methods/prototypes.

I would most definitely expect a forms library to properly support DateTime objects. I'm honestly kinda surprised that it's not supported right now. It's pretty common to have datetime fields in a form.
This pretty much breaks react-hook-form for my me.

I have a form with multiple fields, one of the fields is a DateTimePicker from mui, therefore I have a luxon DateTime object in my form and the .now value as a default. Is this really not supported?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
bluebill1049 pushed a commit that referenced this issue Apr 24, 2023
Co-authored-by: Vic Winberry <vic.winberry@xero.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants